[llvm-debuginfo-analyzer] Use `LLVM_BUILD_DEBUG` in class definitions #140265

jalopezg-git · 2025-05-16T15:06:46Z

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. Specifically, the following is affected by this patch

The aforementioned LVObject::ID
The LVObject::dump() function is virtual, and hence appears in the vtable for class LVObject. Thus, it participates also in class layout and is subject to the same issue.

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

Fixes #139098.

llvmbot · 2025-05-16T15:07:29Z

@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 · 2025-05-16T17:24:58Z

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

Some LLVM libraries may conditionally define some class members depending on whether LLVM build type == Debug. However, relying on `NDEBUG` for this may create situations in which the consumer of a header in a downstream project has a different definition for `NDEBUG`, resulting in an ODR violation. The use of `LLVM_BUILD_DEBUG` should be preferred in these cases.

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. See llvm#139098 for details. Instead, rely on `LLVM_BUILD_DEBUG` for class definitions.

jalopezg-git · 2025-05-18T14:01:30Z

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 · 2025-05-19T00:21:30Z

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 · 2025-05-19T10:23:28Z

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 · 2025-05-19T15:54:39Z

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 · 2025-05-19T17:12:39Z

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 · 2025-05-19T18:54:44Z

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 · 2025-05-20T06:17:05Z

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 · 2025-05-20T10:24:31Z

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 · 2025-05-20T23:17:30Z

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 · 2025-05-21T10:32:02Z

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?

llvmbot added the debuginfo label May 16, 2025

jalopezg-git force-pushed the jalopezg-fix-LVObject-odr-violation branch 2 times, most recently from 0301469 to 246f933 Compare May 16, 2025 16:41

jalopezg-git force-pushed the jalopezg-fix-LVObject-odr-violation branch 2 times, most recently from f237c52 to 11bec1f Compare May 17, 2025 14:18

jalopezg-git added 2 commits May 17, 2025 16:27

jalopezg-git force-pushed the jalopezg-fix-LVObject-odr-violation branch from 11bec1f to 0f6d6ea Compare May 17, 2025 14:27

CarlosAlbertoEnciso assigned jalopezg-git May 19, 2025

CarlosAlbertoEnciso added llvm:binary-utilities tools:llvm-debuginfo-analyzer labels May 19, 2025

CarlosAlbertoEnciso requested review from jmorse, SLTozer, OCHyams and CarlosAlbertoEnciso May 19, 2025 11:56

compnerd self-requested a review May 19, 2025 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-debuginfo-analyzer] Use `LLVM_BUILD_DEBUG` in class definitions #140265

[llvm-debuginfo-analyzer] Use `LLVM_BUILD_DEBUG` in class definitions #140265

jalopezg-git commented May 16, 2025 •

edited

Loading

llvmbot commented May 16, 2025 •

edited

Loading

dwblaikie commented May 16, 2025

jalopezg-git commented May 18, 2025

dwblaikie commented May 19, 2025

jalopezg-git commented May 19, 2025 •

edited

Loading

compnerd commented May 19, 2025

jalopezg-git commented May 19, 2025

compnerd commented May 19, 2025

CarlosAlbertoEnciso commented May 20, 2025

jalopezg-git commented May 20, 2025 •

edited

Loading

compnerd commented May 20, 2025

jalopezg-git commented May 21, 2025

[llvm-debuginfo-analyzer] Use LLVM_BUILD_DEBUG in class definitions #140265

Are you sure you want to change the base?

[llvm-debuginfo-analyzer] Use LLVM_BUILD_DEBUG in class definitions #140265

Conversation

jalopezg-git commented May 16, 2025 • edited Loading

llvmbot commented May 16, 2025 • edited Loading

dwblaikie commented May 16, 2025

jalopezg-git commented May 18, 2025

dwblaikie commented May 19, 2025

jalopezg-git commented May 19, 2025 • edited Loading

compnerd commented May 19, 2025

jalopezg-git commented May 19, 2025

compnerd commented May 19, 2025

CarlosAlbertoEnciso commented May 20, 2025

jalopezg-git commented May 20, 2025 • edited Loading

compnerd commented May 20, 2025

jalopezg-git commented May 21, 2025

[llvm-debuginfo-analyzer] Use `LLVM_BUILD_DEBUG` in class definitions #140265

[llvm-debuginfo-analyzer] Use `LLVM_BUILD_DEBUG` in class definitions #140265

jalopezg-git commented May 16, 2025 •

edited

Loading

llvmbot commented May 16, 2025 •

edited

Loading

jalopezg-git commented May 19, 2025 •

edited

Loading

jalopezg-git commented May 20, 2025 •

edited

Loading