[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D97446#3240309 <https://reviews.llvm.org/D97446#3240309>, @JonChesterfield wrote:

> I don't know the context of this patch but changing attribute((used)) to put things under compiler.used is definitely a breaking change. Please introduce a new attribute if necessary for garbage collection purposes instead of breaking this.

This change was implemented so that llvm.used could prevent section GC on ELF, to make its semantics consistent with llvm.used on COFF and MachO. Essentially, llvm.used behaves differently on ELF now so to prevent behavior changes for users of `__attribute__((used))`, it was migrated to `llvm.compiler.used` on ELF targets. This is consistent with GCC's behavior.

This should only change behavior for you if you depend on the details of the LLVM IR being emitted, or perhaps if you use LTO, where GlobalOpt will behave differently. I don't think these use cases are as important as consistent ELF section GC behavior.

So, apologies for changing the LLVM IR emitted for this attribute, but I think it's unlikely we will change our minds and revert this a year later.

> 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.
>
> 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.
>
> 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"?





================
Comment at: clang/lib/CodeGen/CGDecl.cpp:445
   if (D.hasAttr<UsedAttr>())
-    CGM.addUsedGlobal(var);
+    CGM.addUsedOrCompilerUsedGlobal(var);
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446