[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield reopened this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

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.

If I had a red buildbot to reference I'd revert. This breaks 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.



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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446