[JSON][NFC] Move print method out of NDEBUG || DUMP

[npanchen]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Nikolay Panchenko (npanchen)

Changes

---

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

2 Files Affected:

• (modified) llvm/include/llvm/Support/JSON.h (+1-1)
• (modified) llvm/lib/Support/JSON.cpp (-2)

diff --git a/llvm/include/llvm/Support/JSON.h b/llvm/include/llvm/Support/JSON.h
index 3e3f783ffb857..7f7f5f6228763 100644
--- a/llvm/include/llvm/Support/JSON.h
+++ b/llvm/include/llvm/Support/JSON.h
@@ -472,8 +472,8 @@ class Value {
     return LLVM_LIKELY(Type == T_Array) ? &as<json::Array>() : nullptr;
   }
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   void print(llvm::raw_ostream &OS) const;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   LLVM_DUMP_METHOD void dump() const {
     print(llvm::dbgs());
     llvm::dbgs() << '\n';
diff --git a/llvm/lib/Support/JSON.cpp b/llvm/lib/Support/JSON.cpp
index 98fef5dcf68a3..e7b762f550d46 100644
--- a/llvm/lib/Support/JSON.cpp
+++ b/llvm/lib/Support/JSON.cpp
@@ -182,9 +182,7 @@ void Value::destroy() {
   }
 }
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 void Value::print(llvm::raw_ostream &OS) const { OS << *this; }
-#endif // !NDEBUG || LLVM_ENABLE_DUMP
 
 bool operator==(const Value &L, const Value &R) {
   if (L.kind() != R.kind())

[mscuttari]

The patch fixes the linking problem I was mentioning in the previous pull request (Release-build of LLVM + Debug-build of out-of-tree project), so it looks good from me.

I am still not convinced about having this kind of conditional compilation inside a header file (if you do a global search, you will find such an ifdef only in implementation files). However I will let others discuss on this topic, as it may be fine as long as everything works.

@dwblaikie @kuhar adding you as reviewers, as you are listed as maintainers for the Support library.

[npanchen]

I am still not convinced about having this kind of conditional compilation inside a header file (if you do a global search, you will find such an ifdef only in implementation files). However I will let others discuss on this topic, as it may be fine as long as everything works.

Simple grep of LLVM_ENABLE_DUMP shows about 55 cases in llvm/include. Some of them might not relate to dump or print methods. The similar one can be seen in MCAsmMacro.
I'm fine to drop the use of the macro if that functionality seems nice-to-have for release build, otherwise it's better to keep it
under if.

[mscuttari]

Simple grep of LLVM_ENABLE_DUMP shows about 55 cases in llvm/include. Some of them might not relate to dump or print methods. The similar one can be is in MCAsmMacro. I'm fine to drop the use of the macro if that functionality seems nice-to-have for release build, otherwise it's better to keep it under if.

Right, I was wrongly looking under a different, restricted, scope.
In the file you mentioned, the usage is however limited to the dump method, and not to the print method as per the previous PR.
As I was anticipating, it's fine by me as long as it does not impose any build type assumption to downstream users (and this PR fixes such a problem), but let's also hear others people opinion on this.

[dwblaikie]

The patch fixes the linking problem I was mentioning in the previous pull request (Release-build of LLVM + Debug-build of out-of-tree project), so it looks good from me.

Could you link to the artifacts/perhaps provide more details here about the caller, link error, etc?

[mscuttari]

Could you link to the artifacts/perhaps provide more details here about the caller, link error, etc?

Sure, here is a reproducer for the issue that this PR aims to fix: https://gist.github.com/mscuttari/2b69f2bcbc6b0e22527d07eae10ead1e

To reproduce the issue, LLVM needs to be built in Release mode and the project in Debug mode. Of course you also need to specify the LLVM_DIR variable for CMake.

The error is the classic undefined reference message, in this case related to the new print method (see #129302), which was ifdefed together with the dump one.

[npanchen]

Right, I was wrongly looking under a different, restricted, scope. In the file you mentioned, the usage is however limited to the dump method, and not to the print method as per the previous PR. As I was anticipating, it's fine by me as long as it does not impose any build type assumption to downstream users (and this PR fixes such a problem), but let's also hear others people opinion on this.

Well, I don't think naming matters here. What matters is that void dump(raw_ostream &OS) const; is defined in cpp file, i.e. including MCAsmMacro.h in debug build and linking with a release LLVM would be the same problem, right ?

[mscuttari]

Well, I don't think naming matters here. What matters is that void dump(raw_ostream &OS) const; is defined in cpp file, i.e. including MCAsmMacro.h in debug build and linking with a release LLVM would be the same problem, right ?

Right, I've tried and indeed that is also problematic, I guess I have been just lucky up until now not to have ever stumbled across this problem. Imho that and similar headers should also be fixed (maybe by moving the implementation of dump() away from the header file?). Requiring downstream users to match the build type seems exaggerated to me, especially considering the size of the project. Unless I'm missing out on something, of course.

[nikic]

I believe mixing asserts and non-asserts builds is supported in principle, but you need to take care on the exact configuration (LLVM_ENABLE_DUMP and LLVM_ENABLE_ABI_BREAKING_CHECKS need to be configured appropriately on both sides of the fence).

But TBH, I'd be fine if we just generally (across the whole codebase) stopped making dump definitions conditional. I doubt that these take up enough code size for this to be worthwhile optimization. (We can keep the existing behavior that LLVM_DUMP_METHOD is only defined as LLVM_USED in assertions builds, so it can still be optimized out in LTO builds.)

[npanchen]

@nikic @dwblaikie are you ok if I merge this change ?

[kuhar]

print functions shouldn't be guarded with these ifdefs AFAIK, so this looks correct