[llvm-debuginfo-analyzer] Fix ODR violation in llvm::logicalview::LVObject

[jalopezg-git]

Some data members are only part of a class definition in a Debug build, e.g. LVObject::ID. If debuginfologicalview is used as a library, NDEBUG cannot be used for this purpose, as this PP macro may have a different definition in a downstream project, which in turn triggers an ODR violation. Fix it by

• Making LVObject::ID an unconditional data member.
• Making LVObject::dump() non-virtual. Rationale: virtual is not needed (and it calls print(), which is virtual anyway).

Fixes #139098.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-debuginfo

Author: Javier Lopez-Gomez (jalopezg-git)

Changes

Some data members are only part of a class definition in a Debug build, e.g. LVObject::ID. If debuginfologicalview is used as a library, NDEBUG cannot be used for this purpose, as this PP macro may have a different definition in a downstream project, which in turn triggers an ODR violation.

Fix this situation by introducing LLVM_BUILD_DEBUG in llvm-config.h, which will be defined only if LLVM itself was built in Debug mode.

See #139098 for details.

FYI, @CarlosAlbertoEnciso. I think this is ready for review - let me know what you think.

---

Full diff: https://github.com/llvm/llvm-project/pull/140265.diff

17 Files Affected:

• (modified) llvm/CMakeLists.txt (+5)
• (modified) llvm/include/llvm/Config/llvm-config.h.cmake (+3)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h (+2-2)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h (+6-5)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h (+2-2)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h (+1-1)
• (modified) llvm/include/llvm/DebugInfo/LogicalView/Readers/LVDWARFReader.h (+1-1)

diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt index 91bedba8a548d..bb85b9c11da73 100644 --- a/llvm/CMakeLists.txt +++ b/llvm/CMakeLists.txt @@ -492,6 +492,11 @@ set(LLVM_LIBRARY_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}) # --libdir set(LLVM_MAIN_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR} ) # --src-root set(LLVM_MAIN_INCLUDE_DIR ${LLVM_MAIN_SRC_DIR}/include ) # --includedir set(LLVM_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR} ) # --prefix +if( uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" ) + set(LLVM_BUILD_DEBUG ON) +else() + set(LLVM_BUILD_DEBUG OFF) +endif() # Note: LLVM_CMAKE_DIR does not include generated files diff --git a/llvm/include/llvm/Config/llvm-config.h.cmake b/llvm/include/llvm/Config/llvm-config.h.cmake index dbc882937b4f4..3e1ed42e5735c 100644 --- a/llvm/include/llvm/Config/llvm-config.h.cmake +++ b/llvm/include/llvm/Config/llvm-config.h.cmake @@ -104,6 +104,9 @@ /* Define to 1 if you have the <sysexits.h> header file. */ #cmakedefine HAVE_SYSEXITS_H ${HAVE_SYSEXITS_H} +/* Define if this LLVM build is a Debug build */ +#cmakedefine LLVM_BUILD_DEBUG + /* Define if building libLLVM shared library */ #cmakedefine LLVM_BUILD_LLVM_DYLIB diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h index 4019ea6f17448..f881d1cccaa78 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVCompare.h @@ -76,7 +76,7 @@ class LVCompare final { void printItem(LVElement *Element, LVComparePass Pass); void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h index c335c34e372b9..c196fefd95db4 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h @@ -105,7 +105,7 @@ class LVLine : public LVElement { void print(raw_ostream &OS, bool Full = true) const override; void printExtra(raw_ostream &OS, bool Full = true) const override {} -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h index 3b556f9927832..c50924cdc6540 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h @@ -49,7 +49,7 @@ class LVOperation final { void print(raw_ostream &OS, bool Full = true) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() { print(dbgs()); } #endif }; @@ -159,7 +159,7 @@ class LVLocation : public LVObject { void print(raw_ostream &OS, bool Full = true) const override; void printExtra(raw_ostream &OS, bool Full = true) const override; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h index efc8db12a6972..18f5b707f428d 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h @@ -15,6 +15,7 @@ #define LLVM_DEBUGINFO_LOGICALVIEW_CORE_LVOBJECT_H #include "llvm/BinaryFormat/Dwarf.h" +#include "llvm/Config/llvm-config.h" #include "llvm/DebugInfo/CodeView/CodeView.h" #include "llvm/DebugInfo/CodeView/TypeIndex.h" #include "llvm/DebugInfo/LogicalView/Core/LVSupport.h" @@ -154,7 +155,7 @@ class LVObject { // copy constructor to create that object; it is used to print a reference // to another object and in the case of templates, to print its encoded args. LVObject(const LVObject &Object) { -#ifndef NDEBUG +#ifdef LLVM_BUILD_DEBUG incID(); #endif Properties = Object.Properties; @@ -165,7 +166,7 @@ class LVObject { Parent = Object.Parent; } -#ifndef NDEBUG +#ifdef LLVM_BUILD_DEBUG // This is an internal ID used for debugging logical elements. It is used // for cases where an unique offset within the binary input file is not // available. @@ -193,7 +194,7 @@ class LVObject { public: LVObject() { -#ifndef NDEBUG +#ifdef LLVM_BUILD_DEBUG incID(); #endif }; @@ -311,13 +312,13 @@ class LVObject { // (class attributes, debug ranges, files, directories, etc). virtual void printExtra(raw_ostream &OS, bool Full = true) const {} -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) virtual void dump() const { print(dbgs()); } #endif uint64_t getID() const { return -#ifndef NDEBUG +#ifdef LLVM_BUILD_DEBUG ID; #else 0; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h index ac0dfba0f1ede..f9a9580bf7909 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVOptions.h @@ -435,7 +435,7 @@ class LVOptions { void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; @@ -632,7 +632,7 @@ class LVPatterns final { void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h index 3ec0ccb31168f..6790ebc396249 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h @@ -87,7 +87,7 @@ class LVRange final : public LVObject { void print(raw_ostream &OS, bool Full = true) const override; void printExtra(raw_ostream &OS, bool Full = true) const override {} -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h index 9ce26398e48df..f6c96588a0bea 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h @@ -325,7 +325,7 @@ class LVReader { void print(raw_ostream &OS) const; virtual void printRecords(raw_ostream &OS) const {} -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h index 1b3c377cd7dbb..8cf92811f64cd 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h @@ -317,7 +317,7 @@ class LVScope : public LVElement { virtual void printWarnings(raw_ostream &OS, bool Full = true) const {} virtual void printMatchedElements(raw_ostream &OS, bool UseMatchedElements) {} -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h index 8ce751a56c590..432dcc32d55df 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVStringPool.h @@ -80,7 +80,7 @@ class LVStringPool { } } -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h index 25bfa9eb77d8a..df14a7699a658 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h @@ -183,7 +183,7 @@ class LVSymbol final : public LVElement { void print(raw_ostream &OS, bool Full = true) const override; void printExtra(raw_ostream &OS, bool Full = true) const override; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h index 28881b3c95b17..9226fec6eb718 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Core/LVType.h @@ -139,7 +139,7 @@ class LVType : public LVElement { void print(raw_ostream &OS, bool Full = true) const override; void printExtra(raw_ostream &OS, bool Full = true) const override; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const override { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h b/llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h index bf30501d00c1f..23c0e317f771c 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h @@ -88,7 +88,7 @@ class LVReaderHandler { void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h index 9cda64e33ddf7..bb7d837647650 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h @@ -232,7 +232,7 @@ class LVBinaryReader : public LVReader { void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h index 4dd7c967ddc17..285e58a8f63e9 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h @@ -225,7 +225,7 @@ class LVCodeViewReader final : public LVBinaryReader { LogicalVisitor.printRecords(OS); }; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; diff --git a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVDWARFReader.h b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVDWARFReader.h index aa47bd9cd2cdd..8c7278c5f326e 100644 --- a/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVDWARFReader.h +++ b/llvm/include/llvm/DebugInfo/LogicalView/Readers/LVDWARFReader.h @@ -146,7 +146,7 @@ class LVDWARFReader final : public LVBinaryReader { void print(raw_ostream &OS) const; -#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) +#if defined(LLVM_BUILD_DEBUG) || defined(LLVM_ENABLE_DUMP) void dump() const { print(dbgs()); } #endif }; 

[dwblaikie]

I think we already have a separate macro for this - ABI_BREAKING_CHECKS, perhaps?

[jalopezg-git]

I think we already have a separate macro for this - ABI_BREAKING_CHECKS, perhaps?

Hmmm... unsure; according to llvm/docs/ProgrammersManual.rst, this PP macro serves a slightly different purpose, but we could probably also use it here. WDYT, @CarlosAlbertoEnciso?

[dwblaikie]

I think we already have a separate macro for this - ABI_BREAKING_CHECKS, perhaps?

Hmmm... unsure; according to llvm/docs/ProgrammersManual.rst, this PP macro serves a slightly different purpose, but we could probably also use it here. WDYT, @CarlosAlbertoEnciso?

The difference being a debug build V an asserts-ish build? Yeah, I think the right thing to do would be to make these members enabled/disabled based on ABI_BREAKING_CHECKS..
Oh, the issue is these aren't checks? They're only for debug dumping features? That is a bit awkward. Do they have to be ABI breaking to provide debugging features/could they be implemented in some other way? (& I don't think we enable dump functions based on whether it's a debug build, right? There's some separate macro for that?)

Might be worth a discourse post for discussion

[jalopezg-git]

Oh, the issue is these aren't checks? They're only for debug dumping features? That is a bit awkward. Do they have to be ABI breaking to provide debugging features/could they be implemented in some other way? (& I don't think we enable dump functions based on whether it's a debug build, right? There's some separate macro for that?)

Usually dump() functions are kept in most (all?) other LLVM sub-projects, e.g. in Clang and MLIR. We could perhaps do the same without too much penalty.
However, here there is also LVObject::ID, which is only used for debugging, AFAIU. There's also a size concern (that LVObject should be kept to minimum size), and this data member is not needed in non-Debug builds.

Yep, we could also raise a thread on Discourse for further discussion 👍.

[compnerd]

I think we already have a separate macro for this - ABI_BREAKING_CHECKS, perhaps?

The ABI_BREAKING_CHECKS is meant for changes to the data structure layout that changes between Asserts and NoAsserts builds IIRC. The dump methods can be enabled/disabled separately (similar to how building with different targets is not considered ABI breaking).

I don't understand the motivation of this change. I realize that NDEBUG is a confusing macro, but this is not about a debug build. NDEBUG indicates whether assertions are enabled or not.

[jalopezg-git]

I don't understand the motivation of this change. I realize that NDEBUG is a confusing macro, but this is not about a debug build. NDEBUG indicates whether assertions are enabled or not.

Basically, when using debuginfologicalview as a library from a downstream project, NDEBUG may be defined if the downstream project is built in Release mode.
This results in a different class layout (e.g. for LVObject), given that, by the time the LVObject.h is parsed in a downstream project, NDEBUG may be defined (or not) regardless of whether it was defined (or not) when LLVM was built.

Thus, NDEBUG cannot be relied for anything that affects class layout (at least on public headers). Looking at @dwblaikie and @compnerd comments, perhaps using LLVM_ENABLE_ABI_BREAKING_CHECKS (instead of introducing a new LLVM_BUILD_DEBUG PP macro) is the way to go 🤔 (?)

[compnerd]

I don't understand the motivation of this change. I realize that NDEBUG is a confusing macro, but this is not about a debug build. NDEBUG indicates whether assertions are enabled or not.

Basically, when using debuginfologicalview as a library from a downstream project, NDEBUG may be defined if the downstream project is built in Release mode. This results in a different class layout (e.g. for LVObject), given that, by the time the LVObject.h is parsed in a downstream project, NDEBUG may be defined (or not) regardless of whether it was defined (or not) when LLVM was built.

Thus, NDEBUG cannot be relied for anything that affects class layout (at least on public headers). Looking at @dwblaikie and @compnerd comments, perhaps using LLVM_ENABLE_ABI_BREAKING_CHECKS (instead of introducing a new LLVM_BUILD_DEBUG PP macro) is the way to go 🤔 (?)

Well, dump shouldn't be part of the public interface, so removal is a better option. Note the guard - LLVM_ENABLE_DUMP is an alternative spelling. Depending on NDEBUG is acceptable here. This doesn't change the object layout, but does change the API surface. If the downstream project is attempting to use the dump methods, it should be building LLVM with LLVM_ENABLE_DUMP.

[CarlosAlbertoEnciso]

I don't understand the motivation of this change. I realize that NDEBUG is a confusing macro, but this is not about a debug build. NDEBUG indicates whether assertions are enabled or not.

Basically, when using debuginfologicalview as a library from a downstream project, NDEBUG may be defined if the downstream project is built in Release mode. This results in a different class layout (e.g. for LVObject), given that, by the time the LVObject.h is parsed in a downstream project, NDEBUG may be defined (or not) regardless of whether it was defined (or not) when LLVM was built.
Thus, NDEBUG cannot be relied for anything that affects class layout (at least on public headers). Looking at @dwblaikie and @compnerd comments, perhaps using LLVM_ENABLE_ABI_BREAKING_CHECKS (instead of introducing a new LLVM_BUILD_DEBUG PP macro) is the way to go 🤔 (?)

Well, dump shouldn't be part of the public interface, so removal is a better option. Note the guard - LLVM_ENABLE_DUMP is an alternative spelling. Depending on NDEBUG is acceptable here. This doesn't change the object layout, but does change the API surface. If the downstream project is attempting to use the dump methods, it should be building LLVM with LLVM_ENABLE_DUMP.

May be some clarification:

 virtual void print(raw_ostream &OS, bool Full = true) const; #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) virtual void dump() const { print(dbgs()); } #endif 

dump should be called only for debug builds.
print can be called for both builds (debug and non debug).

- Debug build: print(dbgs()) - Non Debug Build: print(outs()); 

The downstream project should use print.

[jalopezg-git]

Well, dump shouldn't be part of the public interface, so removal is a better option. Note the guard - LLVM_ENABLE_DUMP is an alternative spelling. Depending on NDEBUG is acceptable here. This doesn't change the object layout, but does change the API surface. If the downstream project is attempting to use the dump methods, it should be building LLVM with LLVM_ENABLE_DUMP.

To add to @CarlosAlbertoEnciso comment, the original purpose of this PR was to make the LVObject class definition consistent (across both LLVM and downstream projects); specifically,

• LVObject::ID (see LVObject.h:174) non-static data member is conditionally added depending on NDEBUG, which clearly violates the ODR if NDEBUG has a different definition when building a downstream project (actually, that is fairly common).
• While doing this change, I also noticed that LVObject::dump() is declared virtual. I believe that, having it or not, changes how the vtable looks, and thus I included it in this patchset.

[compnerd]

May be some clarification:

 virtual void print(raw_ostream &OS, bool Full = true) const; #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) virtual void dump() const { print(dbgs()); } #endif 

dump should be called only for debug builds. print can be called for both builds (debug and non debug).

I think that you are misreading the condition:

!defined(NDEBUG)
You are focusing on this rather than the rest of the condition:

|| defined(LLVM_ENABLE_DUMP)
You can always build LLVM with -D LLVM_ENABLE_DUMP being passed to CMake to enable the dump API if you want.

- Debug build: print(dbgs()) - Non Debug Build: print(outs()); 

The downstream project should use print.

This is also an option that is available to downstream projects.

• LVObject::ID (see LVObject.h:174) non-static data member is conditionally added depending on NDEBUG, which clearly violates the ODR if NDEBUG has a different definition when building a downstream project (actually, that is fairly common).

That is something to guard with LLVM_ENABLE_ABI_BREAKING_CHANGES as that is changing the object layout itself between release and debug builds. However, the returning of the value between 0 and ID is something that can remain as NDEBUG only.

[jalopezg-git]

That is something to guard with LLVM_ENABLE_ABI_BREAKING_CHANGES as that is changing the object layout itself between release and debug builds. However, the returning of the value between 0 and ID is something that can remain as NDEBUG only.

LGTM; with LLVM_BUILD_DEBUG, I tried to mimic the most probable original intent of NDEBUG, but in a way that doesn't break downstream projects.
Going for LLVM_ENABLE_ABI_BREAKING_CHECKS looks also good and I can change it now. Do we have general consensus?

[jalopezg-git]

That is something to guard with LLVM_ENABLE_ABI_BREAKING_CHANGES as that is changing the object layout itself between release and debug builds. However, the returning of the value between 0 and ID is something that can remain as NDEBUG only.

LGTM; with LLVM_BUILD_DEBUG, I tried to mimic the most probable original intent of NDEBUG, but in a way that doesn't break downstream projects. Going for LLVM_ENABLE_ABI_BREAKING_CHECKS looks also good and I can change it now. Do we have general consensus?

I have dropped the original patch and went for

• Regarding LVObject::ID, temporarily went for #if !defined(NDEBUG) && LLVM_ENABLE_ABI_BREAKING_CHECKS (instead of plain #ifndef NDEBUG). This should fix the original issue.
  @CarlosAlbertoEnciso and I are still discussing whether ID should be made unconditional (and smaller; currently uint64_t) instead.
• Making LVObject::dump() non-virtual. Rationale: virtual is not needed (and it calls print(), which is virtual anyway).

[compnerd]

The changes for dump I think make sense. I'm not fully convinced about the remainder of the change. I think that we do want the ID checking in the debug builds (it is meant to be a debugging tool). If we are building without assertions, then we can enable it with ABI breaking changes. That is, I think that #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES is the right approach.

[jalopezg-git]

The changes for dump I think make sense. I'm not fully convinced about the remainder of the change. I think that we do want the ID checking in the debug builds (it is meant to be a debugging tool). If we are building without assertions, then we can enable it with ABI breaking changes. That is, I think that #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES is the right approach.

Thanks for the review, @compnerd!

Note that, although the pull-request mentions [llvm-debuginfo-analyzer], the issue is really in DebugInfoLogicalView (the underlying library used by llvm-debuginfo-analyzer). As described in #139098, the LVObject class layout may differ if the NDEBUG PP macro has different definition (other than it had when LLVM was built) in a downstream project that consumes the library.
Because in a downstream project, NDEBUG is usually used to indicate a Release build, using #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES, may still cause issues from the PoV of the consumer project.

[CarlosAlbertoEnciso]

The changes for dump I think make sense. I'm not fully convinced about the remainder of the change. I think that we do want the ID checking in the debug builds (it is meant to be a debugging tool). If we are building without assertions, then we can enable it with ABI breaking changes. That is, I think that #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES is the right approach.

Thanks for the review, @compnerd!

Note that, although the pull-request mentions [llvm-debuginfo-analyzer], the issue is really in DebugInfoLogicalView (the underlying library used by llvm-debuginfo-analyzer). As described in #139098, the LVObject class layout may differ if the NDEBUG PP macro has different definition (other than it had when LLVM was built) in a downstream project that consumes the library. Because in a downstream project, NDEBUG is usually used to indicate a Release build, using #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES, may still cause issues from the PoV of the consumer project.

Looking at the other --internal=xxx options, the only one that is not available in non-debug builds is --internal=id, which introduce an inconsistence and restriction to the options. I understand what @compnerd mention about the ID, but as @jalopezg-git states, the issue is with the DebugInfoLogicalView library.

The LVObject::ScopeLevel is declared as uint32_t. Maybe we can reduce it and allow ID for all builds.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jalopezg-git]

The changes for dump I think make sense. I'm not fully convinced about the remainder of the change. I think that we do want the ID checking in the debug builds (it is meant to be a debugging tool). If we are building without assertions, then we can enable it with ABI breaking changes. That is, I think that #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES is the right approach.

Thanks for the review, @compnerd!
Note that, although the pull-request mentions [llvm-debuginfo-analyzer], the issue is really in DebugInfoLogicalView (the underlying library used by llvm-debuginfo-analyzer). As described in #139098, the LVObject class layout may differ if the NDEBUG PP macro has different definition (other than it had when LLVM was built) in a downstream project that consumes the library. Because in a downstream project, NDEBUG is usually used to indicate a Release build, using #if !defined(NDEBUG) || ENABLE_ABI_BREAKING_CHANGES, may still cause issues from the PoV of the consumer project.

Looking at the other --internal=xxx options, the only one that is not available in non-debug builds is --internal=id, which introduce an inconsistence and restriction to the options. I understand what @compnerd mention about the ID, but as @jalopezg-git states, the issue is with the DebugInfoLogicalView library.

The LVObject::ScopeLevel is declared as uint32_t. Maybe we can reduce it and allow ID for all builds.

I changed the type of LVObject::ScopeLevel to uint16_t, and LVObject::ID to uint32_t (which, IIRC, it's more that enough even for parsing really large DWARF files, e.g. Chrome debug info). This together w/ careful reorder of some fields in order to avoid accidental padding, leaves sizeof(LVObject) unchanged after making ID an unconditional member.

PTAL, @CarlosAlbertoEnciso. CC: @compnerd.

[compnerd]

Taking advantage of the padding sounds good to me.

[jalopezg-git]

Thanks, @CarlosAlbertoEnciso, for the additional review; as there is consensus, I'll merge the change.