Skip to content

[llvm] revert preprocessor dump method guards from llvm::ScaledNumber #140574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 19, 2025

Conversation

andrurogerz
Copy link
Contributor

@andrurogerz andrurogerz commented May 19, 2025

Purpose

Reverts the preprocessor guards added to the dump() methods in llvm/Support/ScaledNumber.h in #139938 so that the header can be included when building an external project in debug mode against a release LLVM build.

Overview

This is a clean revert of two files modified in #139938. The rest of that change should not cause similar problems.

Background

The following new build error was reported on #139938, which was merged last week:

module.cpp:(.text._ZNK4llvm12ScaledNumberImE4dumpEv[_ZNK4llvm12ScaledNumberImE4dumpEv]+0x34): undefined reference to `llvm::ScaledNumberBase::dump(unsigned long, short, int)'

See further discussion on #139938.

Validation

Implemented a simple external LLVM project to reproduce the issue.
Verified the the following link failure is observed on LLVM main (Windows + Clang) without this change:

C:\WINDOWS\system32\cmd.exe /C "cd . && C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE -nostartfiles -nostdlib -O0 -g -Xclang -gcodeview -D_DEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrtd -Xlinker /subsystem:console  -fuse-ld=lld-link CMakeFiles/llvm-dump-test.dir/main.cxx.obj -o llvm-dump-test.exe -Xlinker /MANIFEST:EMBED -Xlinker /implib:llvm-dump-test.lib -Xlinker /pdb:llvm-dump-test.pdb -Xlinker /version:0.0   -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -loldnames  && cd ."
lld-link: error: undefined symbol: public: static void __cdecl llvm::ScaledNumberBase::dump(unsigned __int64, short, int)
>>> referenced by S:\llvm\llvm-project\llvm\include\llvm\Support\ScaledNumber.h:614
>>>               CMakeFiles/llvm-dump-test.dir/main.cxx.obj:(public: void __cdecl llvm::ScaledNumber<unsigned __int64>::dump(void) const)
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Verified the link issue is resolved after applying this change.

Also, manually included all header files that were modified in #139938 in the test program and verified there are no other link errors.

@llvmbot
Copy link
Member

llvmbot commented May 19, 2025

@llvm/pr-subscribers-llvm-support

Author: Andrew Rogers (andrurogerz)

Changes

Purpose

Reverts the preprocessor guards added to the dump() methods in llvm/Support/ScaledNumber.h in #139938 so that the header can be included when building an external project in debug mode against a release LLVM build.

Overview

This is a clean revert of two files modified in #139938. The rest of that change should not cause similar problems.

Background

The following new build error was reported on #139938, which was merged last week:

module.cpp:(.text._ZNK4llvm12ScaledNumberImE4dumpEv[_ZNK4llvm12ScaledNumberImE4dumpEv]+0x34): undefined reference to `llvm::ScaledNumberBase::dump(unsigned long, short, int)'

See further discussion on #139938.

Validation

Implemented a simple external LLVM project to reproduce the issue.
Verified the the following link failure is observed on LLVM main (Windows + Clang) without this change:

C:\WINDOWS\system32\cmd.exe /C "cd . &amp;&amp; C:\PROGRA~1\LLVM\bin\CLANG_~1.EXE -nostartfiles -nostdlib -O0 -g -Xclang -gcodeview -D_DEBUG -D_DLL -D_MT -Xclang --dependent-lib=msvcrtd -Xlinker /subsystem:console  -fuse-ld=lld-link CMakeFiles/llvm-dump-test.dir/main.cxx.obj -o llvm-dump-test.exe -Xlinker /MANIFEST:EMBED -Xlinker /implib:llvm-dump-test.lib -Xlinker /pdb:llvm-dump-test.pdb -Xlinker /version:0.0   -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -loldnames  &amp;&amp; cd ."
lld-link: error: undefined symbol: public: static void __cdecl llvm::ScaledNumberBase::dump(unsigned __int64, short, int)
&gt;&gt;&gt; referenced by S:\llvm\llvm-project\llvm\include\llvm\Support\ScaledNumber.h:614
&gt;&gt;&gt;               CMakeFiles/llvm-dump-test.dir/main.cxx.obj:(public: void __cdecl llvm::ScaledNumber&lt;unsigned __int64&gt;::dump(void) const)
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Verified the link issue is resolved after applying this change.

Also, manually included all header files modified in #139938 and ensured there were no other link errors.


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

2 Files Affected:

  • (modified) llvm/include/llvm/Support/ScaledNumber.h (+2-10)
  • (modified) llvm/lib/Support/ScaledNumber.cpp (+1-3)
diff --git a/llvm/include/llvm/Support/ScaledNumber.h b/llvm/include/llvm/Support/ScaledNumber.h
index 3d38677f0eb61..87a56809976a3 100644
--- a/llvm/include/llvm/Support/ScaledNumber.h
+++ b/llvm/include/llvm/Support/ScaledNumber.h
@@ -424,10 +424,7 @@ class ScaledNumberBase {
 public:
   static constexpr int DefaultPrecision = 10;
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  LLVM_DUMP_METHOD static void dump(uint64_t D, int16_t E, int Width);
-#endif
-
+  LLVM_ABI static void dump(uint64_t D, int16_t E, int Width);
   LLVM_ABI static raw_ostream &print(raw_ostream &OS, uint64_t D, int16_t E,
                                      int Width, unsigned Precision);
   LLVM_ABI static std::string toString(uint64_t D, int16_t E, int Width,
@@ -610,12 +607,7 @@ template <class DigitsT> class ScaledNumber : ScaledNumberBase {
                      unsigned Precision = DefaultPrecision) const {
     return ScaledNumberBase::print(OS, Digits, Scale, Width, Precision);
   }
-
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-  LLVM_DUMP_METHOD void dump() const {
-    return ScaledNumberBase::dump(Digits, Scale, Width);
-  }
-#endif
+  void dump() const { return ScaledNumberBase::dump(Digits, Scale, Width); }
 
   ScaledNumber &operator+=(const ScaledNumber &X) {
     std::tie(Digits, Scale) =
diff --git a/llvm/lib/Support/ScaledNumber.cpp b/llvm/lib/Support/ScaledNumber.cpp
index 33e8cc3030873..85d7afbea5c69 100644
--- a/llvm/lib/Support/ScaledNumber.cpp
+++ b/llvm/lib/Support/ScaledNumber.cpp
@@ -317,9 +317,7 @@ raw_ostream &ScaledNumberBase::print(raw_ostream &OS, uint64_t D, int16_t E,
   return OS << toString(D, E, Width, Precision);
 }
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void ScaledNumberBase::dump(uint64_t D, int16_t E, int Width) {
+void ScaledNumberBase::dump(uint64_t D, int16_t E, int Width) {
   print(dbgs(), D, E, Width, 0) << "[" << Width << ":" << D << "*2^" << E
                                 << "]";
 }
-#endif

Copy link
Contributor

@hvdijk hvdijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@compnerd compnerd merged commit dd0a1c5 into llvm:main May 19, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants