Add __attribute__((retain)) to LLVM_DUMP_METHOD

[MatzeB]

Without the retain attribute the dump functions will be stripped when
LLVM is compiled with -ffunction-section -Wl,--gc-sections on
ELF-based systems.

[MatzeB]

Some of our internal builds use -ffunction-sections -Wl,--gc-sections and I just noticed that we need an additional __attribute__((retain)) to keep the LLVM dump functions around.

Is there any downside to using this attribute in LLVM?

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Matthias Braun (MatzeB)

Changes

Without the retain attribute the dump functions will be stripped when
LLVM is compiled with -ffunction-section -Wl,--gc-sections on
ELF-based systems.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Support/Compiler.h (+3-1)

diff --git a/llvm/include/llvm/Support/Compiler.h b/llvm/include/llvm/Support/Compiler.h
index dc8b5389069eb..b6d06238f8d83 100644
--- a/llvm/include/llvm/Support/Compiler.h
+++ b/llvm/include/llvm/Support/Compiler.h
@@ -224,7 +224,9 @@
 #define LLVM_PREFETCH(addr, rw, locality)
 #endif
 
-#if __has_attribute(used)
+#if __has_attribute(used) && __has_attribute(retain)
+#define LLVM_ATTRIBUTE_USED __attribute__((__used__, __retain__))
+#elif __has_attribute(used)
 #define LLVM_ATTRIBUTE_USED __attribute__((__used__))
 #else
 #define LLVM_ATTRIBUTE_USED

[MaskRay]

Some of our internal builds use -ffunction-sections -Wl,--gc-sections and I just noticed that we need an additional __attribute__((retain)) to keep the LLVM dump functions around.

Is there any downside to using this attribute in LLVM?

Perhaps we should just add retained to LLVM_DUMP_METHOD?

This will enable USED functions to be garbage-collectable on ELF platforms, where this behavior has been consistently present.

[MatzeB]

Are you saying you rather have a separate macro for retain? Updated the PR to do that.

[MaskRay]

LGTM, the title needs an adjustment now.

[aleks-tmb]

Hi @MatzeB, this change triggered multiple ‘retain’ attribute ignored warnings when building LLVM with gcc-13:

$ gcc --version
gcc (Ubuntu 13.1.0-8ubuntu1~20.04.2) 13.1.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

...
In file included from /home/apopov/llvm-project/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h:26,
                 from /home/apopov/llvm-project/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp:9:
/home/apopov/llvm-project/llvm/include/llvm/CodeGenTypes/LowLevelType.h:288:32: warning: ‘retain’ attribute ignored [-Wattributes]
  288 |   LLVM_DUMP_METHOD void dump() const;
      |                                ^~~~~
In file included from /home/apopov/llvm-project/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h:26,
                 from /home/apopov/llvm-project/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTableExecutorEmitter.cpp:10:
/home/apopov/llvm-project/llvm/include/llvm/CodeGenTypes/LowLevelType.h:288:32: warning: ‘retain’ attribute ignored [-Wattributes]
  288 |   LLVM_DUMP_METHOD void dump() const;

[danilaml]

Above breaks treat-warnings-as-errors builds. Not sure if it's due to incorrect attribute usage or due to this (or similar) gcc "bug": https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

[MatzeB]

Indeed that GCC bug would be a problem. I could publish a follow-up that requires GCC >= 14 to use the retain attribute, do you know if that would be sufficient here?

[MatzeB]

Sorry re-reading the bug-report this it is not a GCC bug that got fixed. And it's a problem that only affects some GCC setups. So seems we have to disable the use of retain with GCC or dial down the warning flags...

It seems _Pragma cannot be inserted in the middle of a type declaration, so I can't switch the diagnostics on/off just around the retain attribute. Guess I will just disable the retain attribute for GCC then. Assuming only a small subset of people benefit from retain and maybe they overlap with the set of people using clang. Otherwise we would need to more broadly disable -Wattributes for GCC which I suspect is less desirable.

[MatzeB]

@aleks-tmb @danilaml @MaskRay Go with #133793 then?

Just dropping the attribute for GCC builds for now... (I suspect there is more folks building with -Werror or folks wanting -Wattributes enabled than folks who actually need the retain attribute given that we haven't had it in LLVM until last week).

[slydiman]

Note a side effect in lldb-server #132274 (comment)
I recently reduced the size of lldb-server (Linux AArch64) from 47MB to 17MB and was going to reduce it to 8MB. But now it is impossible. Any suggestions?

[JDevlieghere]

Note a side effect in lldb-server #132274 (comment) I recently reduced the size of lldb-server (Linux AArch64) from 47MB to 17MB and was going to reduce it to 8MB. But now it is impossible. Any suggestions?

The dump methods should only exist in assert builds. Do you ship lldb-server with assert enabled?

[MatzeB]

Are you disabling the dumpers already? The dump functions can be independently disabled via cmake -DLLVM_ENABLE_DUMP=OFF (they should be off by default for release builds and on by the default for assertion-enabled builds). I assume you don't need those dumper functions when you create release packages for lldb (and if its not for releases, why do you care about the size)?

(I guess it's possible some code in LLVM forgot to check for the LLVM_ENABLE_DUMP macro, but that should be considered a bug and should be easy to fix/submit patches for).

[MatzeB]

Though even if code did forget to test for LLVM_ENABLE_DUMP you would still not get the retain attribute, so should not be worse than before my patch.

[danilaml]

@MatzeB comment says

/// Note that you should also surround dump() functions with
/// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always
/// get stripped in release builds

So I assume not all dump methods are behind ifdefs. Also, wouldn't surprise me if some dump-like functions are annotated with LLVM_ENABLE_DUMP so that they would be available in assertion-enabled builds for gdb but they are still used in release builds (so the macro is used so that there would be a definition debugger could use, rather than complaining that it was inlined).

[slydiman]

The dump methods should only exist in assert builds. Do you ship lldb-server with assert enabled?

We are using release version with asserts on our buildbots. Disabling asserts solved the problem. Thanks.