[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D97446#3241142 <https://reviews.llvm.org/D97446#3241142>, @rnk wrote:

> In D97446#3240309 <https://reviews.llvm.org/D97446#3240309>, @JonChesterfield wrote:
>
>> If I had a red buildbot to reference I would have reverted - the commit message does not make it clear that this is a breaking change. This broke a debugging hook in openmp, which is apparently not tested, and will break any application code that uses compiler.used on a target that uses elf.

llvm.compiler.used hasn't been changed.

>> Linked docs at https://reviews.llvm.org/D97447 suggest applications can get back to a working state by marking things as attribute((used)) attribute((retain)), presumably guarded by a test for whether retain exists. I think this would be another point in a codebase that has to distinguish between gcc and clang versions when picking attributes.
>>
>> edit: further reading suggests retain turned up in gcc 11 and requires binutils 2.36, with semantics similar to but distinct from used. It's a linker section discard directive, so the 'garbage collection roots' in this context may refer to bits of an elf instead of the language runtime feature.

The text focuses on the semantics, and for practical reasons refers to the toolchain support.
Before GCC 11/binutils 2.36 there was just no portable way making a definition not discarded by ld --gc-sections.
The patch series added the support but obviously cannot alter the fact that the toolchain was catching up.

>> I have fixed openmp by marking variables as retain but breaking applications that are relying on used (by discarding the variables) remains bad. Is this breakage already shipping with gcc, thus the ship has sailed, or can we keep backwards compat here?
>>
>> edit: Would making attribute((used)) imply attribute((retain)) on elf targets achieve the objective of this patch without breaking code that expects 'used' to mean "don't throw this away"?

That would make semantics less orthogonal and incompatible with GCC.
On COFF and Mach-O, there have been use cases relying on attribute((used)) implying GC roots.
On ELF, there was none before the toolchain support.
Every llvm.used usage I can find in the wild does intend to have the GC semantics, and not having it on ELF was actually a bug and has been fixed by the patch series.



================
Comment at: clang/lib/CodeGen/CGDecl.cpp:445
   if (D.hasAttr<UsedAttr>())
-    CGM.addUsedGlobal(var);
+    CGM.addUsedOrCompilerUsedGlobal(var);
 
----------------
rnk wrote:
> JonChesterfield wrote:
> > I think this (and the corresponding line in codgen) is incorrect.
> > 
> > Previously, attribute((used)) marked something as 'used', so it makes it all the way to the linker.
> > 
> > After this change, anything that answers getTriple().isOSBinFormatELF() with true will emit ((used)) as compiler.used, which means it gets deleted earlier. In particular, amdgpu uses elf and the openmp runtime marks some symbols used, which are now getting deleted by clang during internalise.
> > 
> > Lots of targets presumably use 'elf' as the target binary format though, so I expect this to have broken user facing code on all of them.
> > 
> This is the same behavior they would get in a native link if they used `-ffunction-sections` / `--gc-sections`, so you have described the desired behavior change: it makes LTO internalize+globalopt consistent with native links.
llvm.used traditionally did not have GC root semantics on ELF platforms, actually it was identical to llvm.compiler.used (modulo possible bugs). This is the intended change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97446/new/

https://reviews.llvm.org/D97446