[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D97446#3241828 , @JonChesterfield 
wrote:

> I'd much refer we put variables in arrays corresponding to their intended 
> lifespan instead of the binary format they're destined for.

I agree that LLVM features should generally mean something consistent across 
targets, and frontends should use the appropriate feature for the semantics 
they require.  Here the problem is that, for better or worse, the source 
language has different semantics on different targets.  Historically, that 
really has been driven by object format and the corresponding presumed linker 
behavior; I don't know where to begin answering the question of whether we 
should treat it as a platform difference instead when targeting a non-standard 
object format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2093
+ "Only globals with definition can force usage.");
+  if (getTriple().isOSBinFormatELF())
+LLVMCompilerUsed.emplace_back(GV);

JonChesterfield wrote:
> ^ this specifically looks wrong, which array the variable goes in should be 
> based on what the variable is used for or what the programmer asked for, not 
> on the binary format used by the OS (is that even a unique test? One can run 
> elf or coff on windows iirc)
`addUsedOrCompilerUsedGlobal` is carefully used on `__attribute__((used))` and 
the related keep-static-consts.cpp.

`__attribute__((used))`  has a semantic split on ELF and non-ELF, so the 
difference is unavoidable. For other constructs we try to use the precise 
LLVMCompilerUsed or LLVMUsed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D97446#3241735 , @MaskRay wrote:

> For your comment it appears an issue in the internalisation pass. It is 
> orthogonal to this patch.
> Do you have a reduced example with reproduce instructions for the issue? I 
> know very little about OpenMP.
> Well, I assume we can continuation the discussion in that OpenMP thread.

Internalize.cpp is fairly clear that it treats the two arrays differently, 
copying the corresponding part. Perhaps orthogonal to but exposed by this patch.

  // We must assume that globals in llvm.used have a reference that not even
  // the linker can see, so we don't internalize them.
  // For llvm.compiler.used the situation is a bit fuzzy. The assembler and
  // linker can drop those symbols. If this pass is running as part of LTO,
  // one might think that it could just drop llvm.compiler.used. The problem
  // is that even in LTO llvm doesn't see every reference. For example,
  // we don't see references from function local inline assembly. To be
  // conservative, we internalize symbols in llvm.compiler.used, but we
  // keep llvm.compiler.used so that the symbol is not deleted by llvm.
  for (GlobalValue *V : Used) {
AlwaysPreserved.insert(V->getName());
  }



>> It's possible that the check in internalize that skips over llvm.used and 
>> not llvm.compiler.used is itself a bug, and the intent is for llvm.used to 
>> be identical to llvm.compiler.used, but if that's the case we should delete 
>> the llvm.compiler.used array.
>
> ...
> I agree that in many cases user don't need more fine-grained GC precision and 
> probably just add both used/retain to not bother think about the problem.
> These people may find the split strange.
>
> llvm.compiler.used (the underlying mechanism of `__attribute__((used))`) is 
> useful in instrumentations.
> There are quite few cases that the compiler does not fully discard 
> definitions and has to defer it to the linker.
> I have changed some instrumentations (PGO/SanitizerCoverage/other sanitizers) 
> to downgrade llvm.used to llvm.compiler.used to improve section based linker 
> garbage collection for all of PE-COFF, Mach-O, and ELF.
> There has been decent object size decrease (at least single digit percent, 
> 10+% for some).

Uh, yes. Discarding things that previously were not discarded will make things 
smaller. The users I'm advocating for here are the ones who would have 
preferred we not discard the things that they asked us to keep and that we used 
to keep. Perhaps they will be few in number, and at least there's a workaround 
to be discovered.

In D97446#3241767 , @rjmccall wrote:

> I can understand how we got here, but it's a bad place to have ended up.
> ...elided...
> At the end of the day, I don't think we have much choice but to follow GCC's 
> lead on ELF platforms.  They get to define what these attributes mean, and if 
> they want to make weaker guarantees on ELF, that's their decision.

That's compelling, thank you for articulating it.

I think we have an outstanding issue to resolve in internalize, where the 
assumption behind this patch that the two forms are equivalent on elf does not 
hold.

I'd much refer we put variables in arrays corresponding to their intended 
lifespan instead of the binary format they're destined for. I don't know if the 
OS in question is certain to match the linker output either, seems possible to 
compile on an OS that usually uses one format but emit code in a different one.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2093
+ "Only globals with definition can force usage.");
+  if (getTriple().isOSBinFormatELF())
+LLVMCompilerUsed.emplace_back(GV);

^ this specifically looks wrong, which array the variable goes in should be 
based on what the variable is used for or what the programmer asked for, not on 
the binary format used by the OS (is that even a unique test? One can run elf 
or coff on windows iirc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I can understand how we got here, but it's a bad place to have ended up.

Toolchains that do dead-stripping need a way to prevent it for specific objects 
and functions.  Windows and Darwin toolchains have historically done aggressive 
dead-stripping in both the compiler and linker, so they've always had this 
need.  ELF toolchains have historically emitted code and restricted the linker 
in ways that, together, largely inhibit link-time dead-stripping (except of 
specific things like C++ entities with vague linkage).  As a result, ELF has 
gotten away with not providing a fine-grained way to prevent link-time 
dead-stripping for a long time, when otherwise that would have been a major 
blocker.

There are basically three broad categories of reasons why programs need to use 
features like this:

1. Some system at runtime needs to be able to find the entity "reflectively" 
(generally, because it's in a section with a special name or will be looked up 
by its symbol name).  Usually this is intended as a kind of passive global 
registration, and so dead-stripping needs to be completely blocked for the 
entity.
2. Some system at runtime that finds the entity "reflectively" will access it 
in weird ways that the compiler can't hope to understand.  The attribute is not 
being used to prevent dead-stripping (unless the entity is *also* a passive 
registration), but to block compiler analysis and optimization if the value 
*does* end up being used.  This tends to be a language-implementation thing 
more than something that a library would do.
3. The entity isn't actually unreferenced, but some portion of the toolchain is 
just unable to see that.  The most important example of this is a reference 
from inline assembly.

If I were a GCC developer reacting to the addition of `retain` to ELF, I would 
have given `__attribute__((used))` the stronger semantics, forcing the entity 
to be retained at link-time, and I would have introduced a weaker attribute (or 
attributes) that people could use selectively in the second and third cases if 
they wanted better dead-stripping.  To me, that's conservatively living up to 
user expectations about what they're trying to achieve with 
`__attribute__((used))` while giving them an opportunity to be more precise if 
it's useful; and it also unifies the semantics across object formats now that 
it's possible to do so.  But that's not generally how GCC developers do things; 
they tend to treat the current compiler behavior as an inviolate contract down 
to a very low level, and if you want different behavior, you need to use 
different options.  I can understand and sympathize with that approach, even if 
I think it also tends to create situations like this one.

At the end of the day, I don't think we have much choice but to follow GCC's 
lead on ELF platforms.  They get to define what these attributes mean, and if 
they want to make weaker guarantees on ELF, that's their decision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D97446#3241617 , @JonChesterfield 
wrote:

>> Modulo optimizer bugs, __attribute__((used)) hasn't changed semantics.
>> If your downstream project does not handle llvm.compiler.used, maybe handle 
>> it now :)
>
> There seems to be some confusion here. The 'downstream project' is openmp, 
> which has worked around this regression in D117211 
>  and D117231 
> .
>
> Before this patch, `__attribute__((used))` mapped onto llvm.used, and the 
> variables so annotated made it all the way to the compiled artefact. After 
> this patch, it is mapped onto llvm.compiler.used, gets hit by an 
> internalisation pass and ends up in the compiled output but missing from the 
> symbol table. That is itself presumably a bug, as the linker should have 
> completely discarded it, with much the same effect.

For your comment it appears an issue in the internalisation pass. It is 
orthogonal to this patch.
Do you have a reduced example with reproduce instructions for the issue? I know 
very little about OpenMP.

> With this patch applied, what's the remaining use case for 
> `__attribute__((used))`? It can no longer be used to keep something in the 
> final executable, so it seems s/used/retain/g is the recommendation for 
> programs that previously used that attribute.
>
> It's possible that the check in internalize that skips over llvm.used and not 
> llvm.compiler.used is itself a bug, and the intent is for llvm.used to be 
> identical to llvm.compiler.used, but if that's the case we should delete the 
> llvm.compiler.used array.

A `__attribute__((used))` object can be referenced by a relocation. The 
relocation (from A to B) establishes a dependency relation in the linker: if 
the section defining A is retained, the section defining B should be retained 
as well.
This can be used by inline assembly (the compiler may not know the symbol 
references in assembly), and some code doing manual relocations.

llvm.compiler.used (the underlying mechanism of `__attribute__((used))`) is 
useful in instrumentations.
There are quite few cases that the compiler does not fully discard definitions 
and has to defer it to the linker.
I have changed some instrumentations (PGO/SanitizerCoverage/other sanitizers) 
to downgrade llvm.used to llvm.compiler.used to improve section based linker 
garbage collection for all of PE-COFF, Mach-O, and ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

> Modulo optimizer bugs, __attribute__((used)) hasn't changed semantics.
> If your downstream project does not handle llvm.compiler.used, maybe handle 
> it now :)

There seems to be some confusion here. The 'downstream project' is openmp, 
which has worked around this regression in D117211 
 and D117231 
.

Before this patch, __attribute__((used)) mapped onto llvm.used, and the 
variables so annotated made it all the way to the compiled artefact. After this 
patch, it is mapped onto llvm.compiler.used, gets hit by an internalisation 
pass and ends up in the compiled output but missing from the symbol table. That 
is itself presumably a bug, as the linker should have completely discarded it, 
with much the same effect.

With this patch applied, what's the remaining use case for 
__attribute__((used))? It can no longer be used to keep something in the final 
executable, so it seems s/sed used/retain/g is the recommendation for programs 
that previously used that attribute.

It's possible that the check in internalize that skips over llvm.used and not 
llvm.compiler.used is itself a bug, and the intent is for llvm.used to be 
identical to llvm.compiler.used, but if that's the case we should delete the 
llvm.compiler.used array.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D97446#3241416 , @JonChesterfield 
wrote:

> It looks deeply wrong to be marking globals as either llvm.used or 
> llvm.compiler.used based on the output file format. It should be based on the 
> (purpose of) the global.
>
> In D97446#3241142 , @rnk wrote:
>
>> 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.
>
> Is this sentence inverted? llvm.used should prevent sections from being 
> discarded. If it doesn't at present, that's a bug in the linker.

I agree with rnk.
The bug can be on the compiler side which does not give enough information to 
the linker.
Before `retain` was added to GCC and binutils supported SHF_GNU_RETAIN, the ELF 
world was in that state.

> llvm.compiler.used should generally be discarded by whatever part of the 
> compiler wanted the variable

The compiler cannot discard it per LangRef 
https://llvm.org/docs/LangRef.html#the-llvm-compiler-used-global-variable

> but if it makes it to the linker, the linker should throw it away. Because it 
> was only used by the compiler. It's possible some users marked things as 
> 'used' but wanted them thrown away, but it seems more likely that users 
> weren't using gc-sections if it broke their application by throwing away 
> things they asked to keep.

Correct.

>> 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.
>
> I'm hopeful I can change your minds now. The current modelling in the 
> compiler doesn't match the stated intent.
>
> In D97446#3241195 , @MaskRay wrote:
>
>> llvm.compiler.used hasn't been changed.
>
> True, but attribute((used)) has. I only noticed this patch because it broke 
> some test cases in rocm, but it'll break some of my own code too. Moving a 
> variable from llvm.used to llvm.compiler.used changes whether internalise 
> skips the variable.

Modulo optimizer bugs, `__attribute__((used))` hasn't changed semantics.
If your downstream project does not handle llvm.compiler.used, maybe handle it 
now :)

I apologize that previous Clang probably very rarely emitted llvm.compiler.used 
and it started to do it more often now.

>> 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.
>
> Sure there was. Don't pass gc-sections to the linker, or don't compile with 
> ffunction-sections to get a close approximation.
>
>>> 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"?
>>
>> This would make semantics less orthogonal and incompatible with GCC.
>> On COFF and Mach-O, there have been Clang-specific (not GCC) 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.
>
> I'm absolutely sure that people mark things as attribute((used)) to stop the 
> toolchain discarding them. I think we're in agreement there, but differ in 
> our assessment of popularity of gc-sections.
>
> Are we missing a category here?
>
> llvm.compiler.used <- the compiler uses the global, and may discard it. If it 
> doesn't, the linker should discard it
> llvm.linker.used <- the linker uses this global, and may discard it. The 
> compiler should leave it alone aside from passing it to the linker
> llvm.used <- some unspecified thing uses the global, the compiler and linker 
> should leave it alone aside from embedding it in the linked output

I think the llvm.compiler.used interpretation diverges from LangRef and how we 
teach optimizers to respect llvm.compiler.used.
Then llvm.linker.used shall not be needed.

> If we have to map 'attribute((used))' onto the new llvm.linker.used and 
> 'attribute((retain))' onto llvm.used that's a shame, but at least it keeps 
> the naming weirdness localised to the language front end.

I have checked the documentation of many GCC 

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

It looks deeply wrong to be marking global as either llvm.used or 
llvm.compiler.used based on the output file format. It should be based on the 
(purpose of) the global.

In D97446#3241142 , @rnk wrote:

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

Is this sentence inverted? llvm.used should prevent sections from being 
discarded. If it doesn't at present, that's a bug in the linker. 
llvm.compiler.used should generally be discarded by whatever part of the 
compiler wanted the variable, but if it makes it to the linker, the linker 
should throw it away. Because it was only used by the compiler. It's possible 
some users marked things as 'used' but wanted them thrown away, but it seems 
more likely that users weren't using gc-sections if it broke their application.
 by throwing away things they asked to keep.

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

I'm hopeful I can change your minds now. The current modelling in the compiler 
doesn't match the stated intent.

In D97446#3241195 , @MaskRay wrote:

> llvm.compiler.used hasn't been changed.

True, but attribute((used)) has.

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

Sure there was. Don't pass gc-sections to the linker, or don't compile with 
ffunction-sections to get a close approximation.

>> 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"?
>
> This would make semantics less orthogonal and incompatible with GCC.
> On COFF and Mach-O, there have been Clang-specific (not GCC) 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.

I'm absolutely sure that people mark things as attribute((used)) to stop the 
toolchain discarding them. I think we're in agreement there, but differ in our 
assessment of popularity of gc-sections.

Are we missing a category here?

llvm.compiler.used <- the compiler uses the global, and may discard it. If it 
doesn't, the linker should discard it
llvm.linker.used <- the linker uses this global, and may discard it. The 
compiler should leave it alone aside from passing it to the linker
llvm.used <- some unspecified thing uses the global, the compiler and linker 
should leave it alone aside from embedding it in the linked output

If we have to map 'attribute((used))' onto the new llvm.linker.used and 
'attribute((retain))' onto llvm.used that's a shame, but at least it keeps the 
naming weirdness localised to the language front end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D97446#3241142 , @rnk wrote:

> In 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())
-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In 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())
-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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())
-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG28cb620321f5: Change some addUsedGlobal to 
addUsedOrCompilerUsedGlobal (authored by MaskRay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/2005-12-04-AttributeUsed.c
  clang/test/CodeGen/attr-msp430.c
  clang/test/CodeGen/attr-target-mv.c
  clang/test/CodeGen/attr-used.c
  clang/test/CodeGen/attr-x86-interrupt.c
  clang/test/CodeGen/keep-static-consts.cpp
  clang/test/CodeGenCUDA/llvm-used.cu
  clang/test/CodeGenCXX/attr-x86-interrupt.cpp
  clang/test/CodeGenCXX/extern-c.cpp

Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,7 +70,7 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.compiler.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
Index: clang/test/CodeGenCXX/attr-x86-interrupt.cpp
===
--- clang/test/CodeGenCXX/attr-x86-interrupt.cpp
+++ clang/test/CodeGenCXX/attr-x86-interrupt.cpp
@@ -17,11 +17,11 @@
 struct St {
 static void foo9(int *a) __attribute__((interrupt)) {}
 };
-// X86_64_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_64_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i64 %{{.+}})
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_64_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
-// X86_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i32 %{{.+}})
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
Index: clang/test/CodeGenCUDA/llvm-used.cu
===
--- clang/test/CodeGenCUDA/llvm-used.cu
+++ clang/test/CodeGenCUDA/llvm-used.cu
@@ -4,5 +4,5 @@
 // Make sure we emit the proper addrspacecast for llvm.used.  PR22383 exposed an
 // issue where we were generating a bitcast instead of an addrspacecast.
 
-// CHECK: @llvm.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
 __attribute__((device)) __attribute__((__used__)) int a[] = {};
Index: clang/test/CodeGen/keep-static-consts.cpp
===
--- clang/test/CodeGen/keep-static-consts.cpp
+++ clang/test/CodeGen/keep-static-consts.cpp
@@ -3,7 +3,7 @@
 // CHECK: @_ZL7srcvers = internal constant [4 x i8] c"xyz\00", align 1
 // CHECK: @_ZL8srcvers2 = internal constant [4 x i8] c"abc\00", align 1
 // CHECK: @_ZL1N = internal constant i32 2, align 4
-// CHECK: @llvm.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL8srcvers2, i32 0, i32 0), i8* bitcast (i32* @_ZL1N to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], 

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 326739.
MaskRay added a comment.

Simplify with `CodeGenModule::getTriple()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/2005-12-04-AttributeUsed.c
  clang/test/CodeGen/attr-msp430.c
  clang/test/CodeGen/attr-target-mv.c
  clang/test/CodeGen/attr-used.c
  clang/test/CodeGen/attr-x86-interrupt.c
  clang/test/CodeGen/keep-static-consts.cpp
  clang/test/CodeGenCUDA/llvm-used.cu
  clang/test/CodeGenCXX/attr-x86-interrupt.cpp
  clang/test/CodeGenCXX/extern-c.cpp

Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,7 +70,7 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.compiler.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
Index: clang/test/CodeGenCXX/attr-x86-interrupt.cpp
===
--- clang/test/CodeGenCXX/attr-x86-interrupt.cpp
+++ clang/test/CodeGenCXX/attr-x86-interrupt.cpp
@@ -17,11 +17,11 @@
 struct St {
 static void foo9(int *a) __attribute__((interrupt)) {}
 };
-// X86_64_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_64_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i64 %{{.+}})
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_64_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
-// X86_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i32 %{{.+}})
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
Index: clang/test/CodeGenCUDA/llvm-used.cu
===
--- clang/test/CodeGenCUDA/llvm-used.cu
+++ clang/test/CodeGenCUDA/llvm-used.cu
@@ -4,5 +4,5 @@
 // Make sure we emit the proper addrspacecast for llvm.used.  PR22383 exposed an
 // issue where we were generating a bitcast instead of an addrspacecast.
 
-// CHECK: @llvm.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
 __attribute__((device)) __attribute__((__used__)) int a[] = {};
Index: clang/test/CodeGen/keep-static-consts.cpp
===
--- clang/test/CodeGen/keep-static-consts.cpp
+++ clang/test/CodeGen/keep-static-consts.cpp
@@ -3,7 +3,7 @@
 // CHECK: @_ZL7srcvers = internal constant [4 x i8] c"xyz\00", align 1
 // CHECK: @_ZL8srcvers2 = internal constant [4 x i8] c"abc\00", align 1
 // CHECK: @_ZL1N = internal constant i32 2, align 4
-// CHECK: @llvm.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL8srcvers2, i32 0, i32 0), i8* bitcast (i32* @_ZL1N to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL8srcvers2, i32 0, i32 0), i8* bitcast (i32* @_ZL1N to i8*)], section "llvm.metadata"
 
 static const char srcvers[] 

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm

> I see that it's currently listed as Undocumented. We should fix that while 
> we're rehashing how this is supposed to work, and clarify what it does on 
> each platform. As I understand it, __attribute__((used)) globals will be GC 
> roots on Mac/Win, but not ELF targets. The mismatch in behavior is 
> unfortunate, but it matches the dominant system compilers on those platforms.

I found the doc update in the other patch:
https://reviews.llvm.org/D97447#change-3MBnnJ2r7Yn8




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2093
+ "Only globals with definition can force usage.");
+  if (this->getTarget().getTriple().isOSBinFormatELF())
+LLVMCompilerUsed.emplace_back(GV);

nit: CGM has a getTriple() helper to shorten this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 326480.
MaskRay edited the summary of this revision.
MaskRay added a comment.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

Adopted rnk's list (Thanks!)
Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/2005-12-04-AttributeUsed.c
  clang/test/CodeGen/attr-msp430.c
  clang/test/CodeGen/attr-target-mv.c
  clang/test/CodeGen/attr-used.c
  clang/test/CodeGen/attr-x86-interrupt.c
  clang/test/CodeGen/keep-static-consts.cpp
  clang/test/CodeGenCUDA/llvm-used.cu
  clang/test/CodeGenCXX/attr-x86-interrupt.cpp
  clang/test/CodeGenCXX/extern-c.cpp

Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,7 +70,7 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.compiler.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
Index: clang/test/CodeGenCXX/attr-x86-interrupt.cpp
===
--- clang/test/CodeGenCXX/attr-x86-interrupt.cpp
+++ clang/test/CodeGenCXX/attr-x86-interrupt.cpp
@@ -17,11 +17,11 @@
 struct St {
 static void foo9(int *a) __attribute__((interrupt)) {}
 };
-// X86_64_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_64_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i64 %{{.+}})
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_64_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
-// X86_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i32 %{{.+}})
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
Index: clang/test/CodeGenCUDA/llvm-used.cu
===
--- clang/test/CodeGenCUDA/llvm-used.cu
+++ clang/test/CodeGenCUDA/llvm-used.cu
@@ -4,5 +4,5 @@
 // Make sure we emit the proper addrspacecast for llvm.used.  PR22383 exposed an
 // issue where we were generating a bitcast instead of an addrspacecast.
 
-// CHECK: @llvm.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
 __attribute__((device)) __attribute__((__used__)) int a[] = {};
Index: clang/test/CodeGen/keep-static-consts.cpp
===
--- clang/test/CodeGen/keep-static-consts.cpp
+++ clang/test/CodeGen/keep-static-consts.cpp
@@ -3,7 +3,7 @@
 // CHECK: @_ZL7srcvers = internal constant [4 x i8] c"xyz\00", align 1
 // CHECK: @_ZL8srcvers2 = internal constant [4 x i8] c"abc\00", align 1
 // CHECK: @_ZL1N = internal constant i32 2, align 4
-// CHECK: @llvm.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL8srcvers2, i32 0, i32 0), i8* bitcast (i32* @_ZL1N to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [4 x i8*] [i8* getelementptr inbounds ([4 x i8], [4 x i8]* @_ZL7srcvers, i32 0, i32 0), i8* bitcast (i32* @b to i8*), i8* getelementptr inbounds ([4 x i8], [4 x 

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think the main user-facing feature here to think about is 
`__attribute__((used))`. I see that it's currently listed as Undocumented. We 
should fix that while we're rehashing how this is supposed to work, and clarify 
what it does on each platform. As I understand it, `__attribute__((used))` 
globals will be GC roots on Mac/Win, but not ELF targets. The mismatch in 
behavior is unfortunate, but it matches the dominant system compilers on those 
platforms.

Here are all the existing addUsedGlobal call sites: 
https://reviews.llvm.org/P8257 I categorize them as:

- ObjC/blocks: this wants GC root semantics, since ObjC mainly runs on Mac.
- MS C++ ABI stuff: wants GC root semantics, no change
- OpenMP: unsure, but GC root semantics probably don't hurt
- CodeGenModule: affected in this patch to *not* use GC root semantics so that 
`__attribute__((used))` behavior remains the same on ELF, plus two other minor 
use cases that don't want GC semantics
- Coverage: Probably want GC root semantics
- CGExpr.cpp: refers to LTO, wants GC root
- CGDeclCXX.cpp: one is MS ABI specific, so yes GC root, one is some other C++ 
init functionality, which should form GC roots (C++ initializers can have side 
effects and must run)
- CGDecl.cpp: Changed in this patch for `__attribute__((used))`

So, looks good, I think you spotted all the things that should change.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1829
 VD->getStorageDuration() == SD_Static)
-  addUsedGlobal(GV);
+  addUsedOrCompilerUsedGlobal(GV);
   }

Should -fkeep-static-consts be getting GC root semantics on Mac/Win? We can 
leave this as behavior preserving, I don't think -fkeep-static-consts is a very 
important feature.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5924
 if (Val && !getModule().getNamedValue(Name->getName()))
-  addUsedGlobal(llvm::GlobalAlias::create(Name->getName(), Val));
+  addUsedOrCompilerUsedGlobal(
+  llvm::GlobalAlias::create(Name->getName(), Val));

This probably doesn't need GC semantics, and should always used 
llvm.compiler.used on all platforms. This alias will be internal, this factory 
function it takes the linkage from the aliasee.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D97446#2587996 , @MaskRay wrote:

> In D97446#2587806 , @probinson wrote:
>
>>> GNU ld has a rule "start_/stop_ references from a live input section retain 
>>> the associated C identifier name sections", which LLD may change in the 
>>> future (D96914 ).
>>
>> The phrasing "may change" implies LLD could eliminate the rule; in fact 
>> D96914  will only add a way to opt-out of 
>> the rule.  You can't eliminate the rule entirely without breaking a lot of 
>> useful cases.
>
> Reworded this sentence. D96914  does add a 
> way to opt-out of the rule. In the future (when toolchains are more mature) 
> we may try. My internal tests and Linux kernel tests are try to state that we 
> may not have many dependent cases.

My guess is that many use-cases for `__start_/__stop_` and `used` sections are 
in test and debugging code; it might not be obvious where a dependency is, and 
it might well be the case that GC-ing these sections will make something 
useless without actively breaking it.  The behavior of `__start_/__stop_` has 
been in place for a very long time, IIUC, and it would be very hard to prove 
that nothing bad will happen if you eliminate that behavior.  I don't think 
"Linux will still boot" is a particularly exhaustive test case.

Rather than just make vague worried noises, I will work on putting up a patch 
with my particular use-case, and we can see whether I am worried over nothing.  
If we're lucky I can do that tomorrow, or it might take another week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D97446#2587806 , @probinson wrote:

>> GNU ld has a rule "start_/stop_ references from a live input section retain 
>> the associated C identifier name sections",
>
> which LLD may change in the future (D96914 ).
>
> The phrasing "may change" implies LLD could eliminate the rule; in fact 
> D96914  will only add a way to opt-out of 
> the rule.  You can't eliminate the rule entirely without breaking a lot of 
> useful cases.

Reworded this sentence. D96914  does add a way 
to opt-out of the rule. In the future (when toolchains are more mature) we may 
try. My internal tests and Linux kernel tests are try to state that we may not 
have many dependent cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

> GNU ld has a rule "start_/stop_ references from a live input section retain 
> the associated C identifier name sections",

which LLD may change in the future (D96914 ).

The phrasing "may change" implies LLD could eliminate the rule; in fact D96914 
 will only add a way to opt-out of the rule.  
You can't eliminate the rule entirely without breaking a lot of useful cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: aaron.ballman, rjmccall, rnk, rsmith.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

An global value in the `llvm.used` list does not have GC root semantics on ELF 
targets.
This will be changed in a subsequent backend patch.

Change some `llvm.used` in the ELF code path to use `llvm.compiler.used` to
prevent undesired GC root semantics.

GNU ld has a rule "__start_/__stop_ references from a live input section retain 
the associated C identifier name sections",
which LLD may change in the future (D96914 ).
For `llvm.used` global values defined in a C identifier name section, keep 
using `llvm.used` so that
the future LLD change will not affect them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97446

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGen/2005-12-04-AttributeUsed.c
  clang/test/CodeGen/attr-msp430.c
  clang/test/CodeGen/attr-target-mv.c
  clang/test/CodeGen/attr-used.c
  clang/test/CodeGen/attr-x86-interrupt.c
  clang/test/CodeGen/keep-static-consts.cpp
  clang/test/CodeGenCUDA/llvm-used.cu
  clang/test/CodeGenCXX/attr-x86-interrupt.cpp
  clang/test/CodeGenCXX/extern-c.cpp

Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64 -emit-llvm %s -o - | FileCheck %s
 namespace foo {
 
 // CHECK-NOT: @a = global
@@ -70,7 +70,7 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.compiler.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
Index: clang/test/CodeGenCXX/attr-x86-interrupt.cpp
===
--- clang/test/CodeGenCXX/attr-x86-interrupt.cpp
+++ clang/test/CodeGenCXX/attr-x86-interrupt.cpp
@@ -17,11 +17,11 @@
 struct St {
 static void foo9(int *a) __attribute__((interrupt)) {}
 };
-// X86_64_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_64_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i64)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i64 %{{.+}})
 // X86_64_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_64_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
-// X86_LINUX: @llvm.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
+// X86_LINUX: @llvm.compiler.used = appending global [3 x i8*] [i8* bitcast (void (i32*, i32)* @{{.*}}foo7{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo8{{.*}} to i8*), i8* bitcast (void (i32*)* @{{.*}}foo9{{.*}} to i8*)], section "llvm.metadata"
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo7{{.*}}(i32* byval(i32) %{{.+}}, i32 %{{.+}})
 // X86_LINUX: define{{.*}} x86_intrcc void @{{.*}}foo8{{.*}}(i32* byval(i32) %{{.+}})
 // X86_LINUX: define linkonce_odr x86_intrcc void @{{.*}}foo9{{.*}}(i32* byval(i32) %{{.+}})
Index: clang/test/CodeGenCUDA/llvm-used.cu
===
--- clang/test/CodeGenCUDA/llvm-used.cu
+++ clang/test/CodeGenCUDA/llvm-used.cu
@@ -4,5 +4,5 @@
 // Make sure we emit the proper addrspacecast for llvm.used.  PR22383 exposed an
 // issue where we were generating a bitcast instead of an addrspacecast.
 
-// CHECK: @llvm.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
+// CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* addrspacecast (i8 addrspace(1)* bitcast ([0 x i32] addrspace(1)* @a to i8 addrspace(1)*) to i8*)], section "llvm.metadata"
 __attribute__((device)) __attribute__((__used__)) int a[] = {};
Index: clang/test/CodeGen/keep-static-consts.cpp
===
--- clang/test/CodeGen/keep-static-consts.cpp
+++