[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay abandoned this revision.
MaskRay added a comment.

Thanks folks for review. Folks are more happy with approach 1 on 
https://lists.llvm.org/pipermail/llvm-dev/2021-February/148760.html , so I am 
abandoning this.

I have copied the documentation and tests to D97447 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

We have two implementation choices.

1. Lift the source language restriction (it is currently discouraged) on 
llvm.compiler.used, let llvm.used use `SHF_GNU_RETAIN` on ELF, and change 
`__attribute__((used))` to use llvm.compiler.used on ELF.
2. Don't touch the backend semantics of llvm.used or llvm.compiler.used. Add a 
metadata `!retain` and let `__attribute__((retain))` use that. This is what has 
been implemented in D96837  and this patch.

I posted https://lists.llvm.org/pipermail/llvm-dev/2021-February/148760.html 
(llvm-dev & cfe-dev) (my first message mentions that I lean to 1. I actually 
lean to 2.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

Aha; attribute `used` *by itself* is not sufficient to preserve sections in the 
output.  But the `__start_/__stop_` symbols implicitly create a reference to 
each of the named sections, and that implicit reference can preserve them in 
the output (assuming gc roots etc).
So, the idea is that attribute `retain` can be used *instead* of the 
`__start_/__stop_` symbols, to preserve sections in the output (with the 
advantage that it will work even for sections that do not have a C-identifier 
name).

Thanks for helping me understand this from a user perspective.  That will be 
important when you go to write the release note for this new attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D96838#2582965 , @probinson wrote:

>> For ELF, `used` does not retain the entity, regardless of this patch.
>
> That statement does not match my experience.

You probably used C identifier name sections with `__attribute__((used, 
section("foo")))`. 
The `__start_foo` reference from a live section retains all input sections 
`foo`: 
https://maskray.me/blog/2021-01-31-metadata-sections-comdat-and-shf-link-order#c-identifier-name-sections

> Note that there are two separate variables documented in the LangRef: 
> `llvm.used` (which corresponds to source attribute `used` and is documented 
> as affecting linker behavior) and `llvm.compiler.used` (which is a rare 
> construct with no corresponding source attribute, and does not affect linker 
> behavior).  So, according to LangRef, source attribute `used` maps to 
> `llvm.used` which is documented as affecting linker behavior, and that's 
> exactly the behavior I am seeing in my project.

As the description of this patch suggests, `llvm.used`has linker behavior 
effects on macOS with all symbols, and Windows with non-local-linkage symbols, 
but not effect on ELF.
The behavior you observed is due to `__start_`/`__stop_` references from live 
input sections. (D96914  
https://lists.llvm.org/pipermail/llvm-dev/2021-February/148682.html)

> But from a user perspective, the IR representation is irrelevant, I don't 
> actually care what the description of `llvm.used` says.  I put attribute 
> `used` on my data, and it is still there in the final executable.  That's how 
> things have worked for a long time, and I am concerned that you are about to 
> break it.

You probably meant the LLD patch D96914 . It 
does not change the default now.

> Note that I am not using groups or comdats or anything else funky; I'm 
> declaring static variables in a custom section, adding attribute `used` and 
> the necessary `__start_/__stop_` symbols, and everything works exactly as I 
> want.

If you use a non-C-identifier-name section, then `__start_` does not work. 
`__attribute__((used))` does not retain it on ELF.

> Maybe there is a different patch that more directly relates to my concern, 
> and probably we should be having this conversation on the llvm-dev thread 
> instead of in a patch that  not many people are paying attention to.  I'm 
> very happy to move the discussion elsewhere if that would help.

So GCC folks' opinions is that `used` is orthogonal to `retain`. From this 
viewpoint, `__attribute__((used))` will be better represented as 
`llvm.compiler.used`, but LangRef currently discourages this usage.
I am a bit busy currently, but if you move the discussion to llvm-dev, I'll be 
happy to discuss it there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

> For ELF, `used` does not retain the entity, regardless of this patch.

That statement does not match my experience.

Note that there are two separate variables documented in the LangRef: 
`llvm.used` (which corresponds to source attribute `used` and is documented as 
affecting linker behavior) and `llvm.compiler.used` (which is a rare construct 
with no corresponding source attribute, and does not affect linker behavior).  
So, according to LangRef, source attribute `used` maps to `llvm.used` which is 
documented as affecting linker behavior, and that's exactly the behavior I am 
seeing in my project.

But from a user perspective, the IR representation is irrelevant, I don't 
actually care what the description of `llvm.used` says.  I put attribute `used` 
on my data, and it is still there in the final executable.  That's how things 
have worked for a long time, and I am concerned that you are about to break it. 
 Note that I am not using groups or comdats or anything else funky; I'm 
declaring static variables in a custom section, adding attribute `used` and the 
necessary `__start_/__stop_` symbols, and everything works exactly as I want.

Maybe there is a different patch that more directly relates to my concern, and 
probably we should be having this conversation on the llvm-dev thread instead 
of in a patch that  not many people are paying attention to.  I'm very happy to 
move the discussion elsewhere if that would help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:98
+This support is available in GNU ``ld`` and ``gold`` as of binutils 2.36, as
+well as in ``ld.lld`` 13.
+  }];

probinson wrote:
> As a user, this seems like a complicated thing to figure out.
> Also it seems to understate the case; if you have a linker that does 
> understand SHF_GNU_RETAIN, then you *have* to use attribute `retain` to get 
> the effect you want; otherwise your code breaks, probably silently.
> And it looks like if you happen to combine an older compiler with a 
> new-enough linker, you can never get it to do what you want.
> 
> I hope I'm wrong about these things, but that's what the docs suggest.
> Also it seems to understate the case; if you have a linker that does 
> understand SHF_GNU_RETAIN, then you *have* to use attribute retain to get the 
> effect you want; otherwise your code breaks, probably silently.

For ELF, `used` does not retain the entity, regardless of this patch.

This means on https://llvm.org/docs/LangRef.html#the-llvm-used-global-variable 
, the "linker" part has been false for ELF, and had been false for COFF until 
2018-01 (https://reviews.llvm.org/rG99f479abcf2c9b36daad04eb91cd0aafa659bb1d).

> If a symbol appears in the @llvm.used list, then the compiler, assembler, and 
> **linker** are required to treat the symbol as if there is a reference to the 
> symbol that it cannot see (which is why they have to be named).

`llvm.compiler.used`'s description has

> This is a rare construct that should only be used in rare circumstances, and 
> should not be exposed to source languages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:98
+This support is available in GNU ``ld`` and ``gold`` as of binutils 2.36, as
+well as in ``ld.lld`` 13.
+  }];

As a user, this seems like a complicated thing to figure out.
Also it seems to understate the case; if you have a linker that does understand 
SHF_GNU_RETAIN, then you *have* to use attribute `retain` to get the effect you 
want; otherwise your code breaks, probably silently.
And it looks like if you happen to combine an older compiler with a new-enough 
linker, you can never get it to do what you want.

I hope I'm wrong about these things, but that's what the docs suggest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

It should be possible to make retain work on COFF for internal linkage things 
by emitting a non-comdat section, even when /Gy / -ffunction/data-sections are 
enabled. That's complicated if the thing being retained is in a comdat group, 
but I think it's doable. If the group is being retained and the leader is 
internal, then all of the sections in the group should be retained by GC, and 
none of them need to be marked comdat, and they won't participate in GC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, both COFF and Mach-O have longstanding ways to protect linker 
dead-stripping, and the compiler already has to manually trigger them on those 
targets for `used`, so it's certainly implementable to also trigger them for 
`retain`-without-`used`.  I just don't think it's a very good feature.  It 
seems to me that the use cases of `retain`-without-`used` all basically boil 
down to "Definition A relies on Definition B in some way that isn't just a 
symbol reference."  `retain`-without-`used` is at best a very rough way of 
achieving that underlying goal of forcing Definition B to be emitted if 
something requires Definition A, and what we really want is a way to express 
that dependence so that the compiler/linker can still strip Definition B if 
Definition A is also stripped.

Also in this space, I think we still don't have a way to express 
`llvm.compiler_used` in the source language, i.e. "feel free to dead-strip this 
if it's never referenced, but if it is referenced, there will be funny uses of 
it that the compiler can't reason about".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from some minor improvements to the documentation.




Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

MaskRay wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > rjmccall wrote:
> > > > MaskRay wrote:
> > > > > aaron.ballman wrote:
> > > > > > MaskRay wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > MaskRay wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Should this be a target-specific attribute as it only has 
> > > > > > > > > > effects on ELF targets?
> > > > > > > > > As I understand it, GCC `retain` is not warned on unsupported 
> > > > > > > > > targets.
> > > > > > > > > 
> > > > > > > > > Regardless of GCC's choice, I think not having a `warning: 
> > > > > > > > > unknown attribute 'retain' ignored [-Wunknown-attributes]` 
> > > > > > > > > diagnostic makes sense. `retain` will be usually used 
> > > > > > > > > together with `used`. In Clang, `used` already has "retain" 
> > > > > > > > > semantics on macOS and Windows (I don't know what they do on 
> > > > > > > > > GCC; GCC folks want orthogonality for ELF and I agree). If 
> > > > > > > > > `retain` is silently ignored on macOS and Windows, then users 
> > > > > > > > > don't need to condition compile for different targets.
> > > > > > > > The other side of that is a user who writes only `retain` and 
> > > > > > > > expects it to do something when it's actually being silently 
> > > > > > > > ignored. While they may usually use it in conjunction with 
> > > > > > > > `used`, I'm more worried about the situation where the user is 
> > > > > > > > possibly confused.
> > > > > > > `retain` without `used` can be used on some external linkage 
> > > > > > > definitions, as well as internal linkage definitions which are 
> > > > > > > referenced by live sections. I agree there could be some 
> > > > > > > confusion, but hope with mccall's suggestion (thanks) the 
> > > > > > > documentation is clear.
> > > > > > If `retain` without `used` is an expected usage pattern, then I 
> > > > > > think we need a diagnostic when we ignore the `retain` attribute. I 
> > > > > > don't think it is reasonable to expect users to read the 
> > > > > > documentation because they won't know that they've misused the 
> > > > > > attribute when we silently ignore it.
> > > > > > 
> > > > > > Alternatively, would it be possible to make `retain` useful on all 
> > > > > > targets? e.g., when `retain` is used by itself on a declaration 
> > > > > > compiled for macOS or Windows, the backend does whatever it would 
> > > > > > normally do for `used`?
> > > > > There is the normal behavior: `__attribute__((retain)) static void 
> > > > > f1() {} // expected-warning {{unused function 'f1'}}`.
> > > > > 
> > > > > Sema.cpp:1227 has the unused diagnostics. There are already many 
> > > > > different versions of diagnostics. Do you suggest another diagnostic 
> > > > > line for `used is needed`? If you think so, I'll need to figure out 
> > > > > how to do that...
> > > > > 
> > > > > Added some tests leveraging the existing diagnostic code.
> > > > We could definitely support retain-without-used with the ELF semantics 
> > > > on all targets if we think they're useful.
> > > I was thinking more along the lines of:
> > > ```
> > > def Retain : InheritableAttr, TargetSpecificAttr {
> > >   ...
> > > }
> > > ```
> > > so that on targets where `retain` is ignored, the user gets an unknown 
> > > attribute diagnostic as with other target-specific attributes. However, I 
> > > see now that the attribute being passed along to the backend in all 
> > > situations and so it isn't really a target-specific attribute in the same 
> > > way as `MSNoVTable` (etc) is. It's not that `retain` isn't sensible on 
> > > those targets, it's that different backends will want to handle the 
> > > attribute differently and that might include it being a harmless noop. Is 
> > > that a correct understanding? If so, then I don't think we need to make 
> > > it target-specific or otherwise diagnose it.
> > This is the case where LangRef definition reflects the state on Mach-O but 
> > not on ELF (and not on PE-COFF before 2018-01).
> > 
> > https://reviews.llvm.org/rG99f479abcf2c9b36daad04eb91cd0aafa659bb1d (CC 
> > @compnerd) (2018-01) is the commit. We could let `!retain` (if we choose to 
> > use `!retain` as the representation) lower to similar `/INCLUDE:` 
> > directives in the `.drectve` section.
> > However, I see now that the attribute being passed along to the backend in 
> > all situations and so it isn't really a target-specific attribute in the 
> > same way as MSNoVTable (etc) is.
> 
> Yes. It depends on whether the backend can translate `!retain` into something 
> affecting linker garbage collection behavior.
> On ELF, it works via `SHF_GNU_RETAIN` (requires bleeding-edge 

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

MaskRay wrote:
> aaron.ballman wrote:
> > rjmccall wrote:
> > > MaskRay wrote:
> > > > aaron.ballman wrote:
> > > > > MaskRay wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > MaskRay wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Should this be a target-specific attribute as it only has 
> > > > > > > > > effects on ELF targets?
> > > > > > > > As I understand it, GCC `retain` is not warned on unsupported 
> > > > > > > > targets.
> > > > > > > > 
> > > > > > > > Regardless of GCC's choice, I think not having a `warning: 
> > > > > > > > unknown attribute 'retain' ignored [-Wunknown-attributes]` 
> > > > > > > > diagnostic makes sense. `retain` will be usually used together 
> > > > > > > > with `used`. In Clang, `used` already has "retain" semantics on 
> > > > > > > > macOS and Windows (I don't know what they do on GCC; GCC folks 
> > > > > > > > want orthogonality for ELF and I agree). If `retain` is 
> > > > > > > > silently ignored on macOS and Windows, then users don't need to 
> > > > > > > > condition compile for different targets.
> > > > > > > The other side of that is a user who writes only `retain` and 
> > > > > > > expects it to do something when it's actually being silently 
> > > > > > > ignored. While they may usually use it in conjunction with 
> > > > > > > `used`, I'm more worried about the situation where the user is 
> > > > > > > possibly confused.
> > > > > > `retain` without `used` can be used on some external linkage 
> > > > > > definitions, as well as internal linkage definitions which are 
> > > > > > referenced by live sections. I agree there could be some confusion, 
> > > > > > but hope with mccall's suggestion (thanks) the documentation is 
> > > > > > clear.
> > > > > If `retain` without `used` is an expected usage pattern, then I think 
> > > > > we need a diagnostic when we ignore the `retain` attribute. I don't 
> > > > > think it is reasonable to expect users to read the documentation 
> > > > > because they won't know that they've misused the attribute when we 
> > > > > silently ignore it.
> > > > > 
> > > > > Alternatively, would it be possible to make `retain` useful on all 
> > > > > targets? e.g., when `retain` is used by itself on a declaration 
> > > > > compiled for macOS or Windows, the backend does whatever it would 
> > > > > normally do for `used`?
> > > > There is the normal behavior: `__attribute__((retain)) static void f1() 
> > > > {} // expected-warning {{unused function 'f1'}}`.
> > > > 
> > > > Sema.cpp:1227 has the unused diagnostics. There are already many 
> > > > different versions of diagnostics. Do you suggest another diagnostic 
> > > > line for `used is needed`? If you think so, I'll need to figure out how 
> > > > to do that...
> > > > 
> > > > Added some tests leveraging the existing diagnostic code.
> > > We could definitely support retain-without-used with the ELF semantics on 
> > > all targets if we think they're useful.
> > I was thinking more along the lines of:
> > ```
> > def Retain : InheritableAttr, TargetSpecificAttr {
> >   ...
> > }
> > ```
> > so that on targets where `retain` is ignored, the user gets an unknown 
> > attribute diagnostic as with other target-specific attributes. However, I 
> > see now that the attribute being passed along to the backend in all 
> > situations and so it isn't really a target-specific attribute in the same 
> > way as `MSNoVTable` (etc) is. It's not that `retain` isn't sensible on 
> > those targets, it's that different backends will want to handle the 
> > attribute differently and that might include it being a harmless noop. Is 
> > that a correct understanding? If so, then I don't think we need to make it 
> > target-specific or otherwise diagnose it.
> This is the case where LangRef definition reflects the state on Mach-O but 
> not on ELF (and not on PE-COFF before 2018-01).
> 
> https://reviews.llvm.org/rG99f479abcf2c9b36daad04eb91cd0aafa659bb1d (CC 
> @compnerd) (2018-01) is the commit. We could let `!retain` (if we choose to 
> use `!retain` as the representation) lower to similar `/INCLUDE:` directives 
> in the `.drectve` section.
> However, I see now that the attribute being passed along to the backend in 
> all situations and so it isn't really a target-specific attribute in the same 
> way as MSNoVTable (etc) is.

Yes. It depends on whether the backend can translate `!retain` into something 
affecting linker garbage collection behavior.
On ELF, it works via `SHF_GNU_RETAIN` (requires bleeding-edge toolchain).
On other binary formats, it is currently a no-op. Further notes:

On COFF, I can make `!retain` work by using `/INCLUDE:` for 
non-local-LLVM-linkage definitions.
On macOS, the maintainer can decide whether it is useful to set 

[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

rjmccall wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > MaskRay wrote:
> > > > aaron.ballman wrote:
> > > > > MaskRay wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Should this be a target-specific attribute as it only has effects 
> > > > > > > on ELF targets?
> > > > > > As I understand it, GCC `retain` is not warned on unsupported 
> > > > > > targets.
> > > > > > 
> > > > > > Regardless of GCC's choice, I think not having a `warning: unknown 
> > > > > > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes 
> > > > > > sense. `retain` will be usually used together with `used`. In 
> > > > > > Clang, `used` already has "retain" semantics on macOS and Windows 
> > > > > > (I don't know what they do on GCC; GCC folks want orthogonality for 
> > > > > > ELF and I agree). If `retain` is silently ignored on macOS and 
> > > > > > Windows, then users don't need to condition compile for different 
> > > > > > targets.
> > > > > The other side of that is a user who writes only `retain` and expects 
> > > > > it to do something when it's actually being silently ignored. While 
> > > > > they may usually use it in conjunction with `used`, I'm more worried 
> > > > > about the situation where the user is possibly confused.
> > > > `retain` without `used` can be used on some external linkage 
> > > > definitions, as well as internal linkage definitions which are 
> > > > referenced by live sections. I agree there could be some confusion, but 
> > > > hope with mccall's suggestion (thanks) the documentation is clear.
> > > If `retain` without `used` is an expected usage pattern, then I think we 
> > > need a diagnostic when we ignore the `retain` attribute. I don't think it 
> > > is reasonable to expect users to read the documentation because they 
> > > won't know that they've misused the attribute when we silently ignore it.
> > > 
> > > Alternatively, would it be possible to make `retain` useful on all 
> > > targets? e.g., when `retain` is used by itself on a declaration compiled 
> > > for macOS or Windows, the backend does whatever it would normally do for 
> > > `used`?
> > There is the normal behavior: `__attribute__((retain)) static void f1() {} 
> > // expected-warning {{unused function 'f1'}}`.
> > 
> > Sema.cpp:1227 has the unused diagnostics. There are already many different 
> > versions of diagnostics. Do you suggest another diagnostic line for `used 
> > is needed`? If you think so, I'll need to figure out how to do that...
> > 
> > Added some tests leveraging the existing diagnostic code.
> We could definitely support retain-without-used with the ELF semantics on all 
> targets if we think they're useful.
I was thinking more along the lines of:
```
def Retain : InheritableAttr, TargetSpecificAttr {
  ...
}
```
so that on targets where `retain` is ignored, the user gets an unknown 
attribute diagnostic as with other target-specific attributes. However, I see 
now that the attribute being passed along to the backend in all situations and 
so it isn't really a target-specific attribute in the same way as `MSNoVTable` 
(etc) is. It's not that `retain` isn't sensible on those targets, it's that 
different backends will want to handle the attribute differently and that might 
include it being a harmless noop. Is that a correct understanding? If so, then 
I don't think we need to make it target-specific or otherwise diagnose it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a subscriber: compnerd.
MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

rjmccall wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > MaskRay wrote:
> > > > aaron.ballman wrote:
> > > > > MaskRay wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Should this be a target-specific attribute as it only has effects 
> > > > > > > on ELF targets?
> > > > > > As I understand it, GCC `retain` is not warned on unsupported 
> > > > > > targets.
> > > > > > 
> > > > > > Regardless of GCC's choice, I think not having a `warning: unknown 
> > > > > > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes 
> > > > > > sense. `retain` will be usually used together with `used`. In 
> > > > > > Clang, `used` already has "retain" semantics on macOS and Windows 
> > > > > > (I don't know what they do on GCC; GCC folks want orthogonality for 
> > > > > > ELF and I agree). If `retain` is silently ignored on macOS and 
> > > > > > Windows, then users don't need to condition compile for different 
> > > > > > targets.
> > > > > The other side of that is a user who writes only `retain` and expects 
> > > > > it to do something when it's actually being silently ignored. While 
> > > > > they may usually use it in conjunction with `used`, I'm more worried 
> > > > > about the situation where the user is possibly confused.
> > > > `retain` without `used` can be used on some external linkage 
> > > > definitions, as well as internal linkage definitions which are 
> > > > referenced by live sections. I agree there could be some confusion, but 
> > > > hope with mccall's suggestion (thanks) the documentation is clear.
> > > If `retain` without `used` is an expected usage pattern, then I think we 
> > > need a diagnostic when we ignore the `retain` attribute. I don't think it 
> > > is reasonable to expect users to read the documentation because they 
> > > won't know that they've misused the attribute when we silently ignore it.
> > > 
> > > Alternatively, would it be possible to make `retain` useful on all 
> > > targets? e.g., when `retain` is used by itself on a declaration compiled 
> > > for macOS or Windows, the backend does whatever it would normally do for 
> > > `used`?
> > There is the normal behavior: `__attribute__((retain)) static void f1() {} 
> > // expected-warning {{unused function 'f1'}}`.
> > 
> > Sema.cpp:1227 has the unused diagnostics. There are already many different 
> > versions of diagnostics. Do you suggest another diagnostic line for `used 
> > is needed`? If you think so, I'll need to figure out how to do that...
> > 
> > Added some tests leveraging the existing diagnostic code.
> We could definitely support retain-without-used with the ELF semantics on all 
> targets if we think they're useful.
This is the case where LangRef definition reflects the state on Mach-O but not 
on ELF (and not on PE-COFF before 2018-01).

https://reviews.llvm.org/rG99f479abcf2c9b36daad04eb91cd0aafa659bb1d (CC 
@compnerd) (2018-01) is the commit. We could let `!retain` (if we choose to use 
`!retain` as the representation) lower to similar `/INCLUDE:` directives in the 
`.drectve` section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

MaskRay wrote:
> aaron.ballman wrote:
> > MaskRay wrote:
> > > aaron.ballman wrote:
> > > > MaskRay wrote:
> > > > > aaron.ballman wrote:
> > > > > > Should this be a target-specific attribute as it only has effects 
> > > > > > on ELF targets?
> > > > > As I understand it, GCC `retain` is not warned on unsupported targets.
> > > > > 
> > > > > Regardless of GCC's choice, I think not having a `warning: unknown 
> > > > > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes 
> > > > > sense. `retain` will be usually used together with `used`. In Clang, 
> > > > > `used` already has "retain" semantics on macOS and Windows (I don't 
> > > > > know what they do on GCC; GCC folks want orthogonality for ELF and I 
> > > > > agree). If `retain` is silently ignored on macOS and Windows, then 
> > > > > users don't need to condition compile for different targets.
> > > > The other side of that is a user who writes only `retain` and expects 
> > > > it to do something when it's actually being silently ignored. While 
> > > > they may usually use it in conjunction with `used`, I'm more worried 
> > > > about the situation where the user is possibly confused.
> > > `retain` without `used` can be used on some external linkage definitions, 
> > > as well as internal linkage definitions which are referenced by live 
> > > sections. I agree there could be some confusion, but hope with mccall's 
> > > suggestion (thanks) the documentation is clear.
> > If `retain` without `used` is an expected usage pattern, then I think we 
> > need a diagnostic when we ignore the `retain` attribute. I don't think it 
> > is reasonable to expect users to read the documentation because they won't 
> > know that they've misused the attribute when we silently ignore it.
> > 
> > Alternatively, would it be possible to make `retain` useful on all targets? 
> > e.g., when `retain` is used by itself on a declaration compiled for macOS 
> > or Windows, the backend does whatever it would normally do for `used`?
> There is the normal behavior: `__attribute__((retain)) static void f1() {} // 
> expected-warning {{unused function 'f1'}}`.
> 
> Sema.cpp:1227 has the unused diagnostics. There are already many different 
> versions of diagnostics. Do you suggest another diagnostic line for `used is 
> needed`? If you think so, I'll need to figure out how to do that...
> 
> Added some tests leveraging the existing diagnostic code.
We could definitely support retain-without-used with the ELF semantics on all 
targets if we think they're useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

aaron.ballman wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > MaskRay wrote:
> > > > aaron.ballman wrote:
> > > > > Should this be a target-specific attribute as it only has effects on 
> > > > > ELF targets?
> > > > As I understand it, GCC `retain` is not warned on unsupported targets.
> > > > 
> > > > Regardless of GCC's choice, I think not having a `warning: unknown 
> > > > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes 
> > > > sense. `retain` will be usually used together with `used`. In Clang, 
> > > > `used` already has "retain" semantics on macOS and Windows (I don't 
> > > > know what they do on GCC; GCC folks want orthogonality for ELF and I 
> > > > agree). If `retain` is silently ignored on macOS and Windows, then 
> > > > users don't need to condition compile for different targets.
> > > The other side of that is a user who writes only `retain` and expects it 
> > > to do something when it's actually being silently ignored. While they may 
> > > usually use it in conjunction with `used`, I'm more worried about the 
> > > situation where the user is possibly confused.
> > `retain` without `used` can be used on some external linkage definitions, 
> > as well as internal linkage definitions which are referenced by live 
> > sections. I agree there could be some confusion, but hope with mccall's 
> > suggestion (thanks) the documentation is clear.
> If `retain` without `used` is an expected usage pattern, then I think we need 
> a diagnostic when we ignore the `retain` attribute. I don't think it is 
> reasonable to expect users to read the documentation because they won't know 
> that they've misused the attribute when we silently ignore it.
> 
> Alternatively, would it be possible to make `retain` useful on all targets? 
> e.g., when `retain` is used by itself on a declaration compiled for macOS or 
> Windows, the backend does whatever it would normally do for `used`?
There is the normal behavior: `__attribute__((retain)) static void f1() {} // 
expected-warning {{unused function 'f1'}}`.

Sema.cpp:1227 has the unused diagnostics. There are already many different 
versions of diagnostics. Do you suggest another diagnostic line for `used is 
needed`? If you think so, I'll need to figure out how to do that...

Added some tests leveraging the existing diagnostic code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

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

Add tests that   Sema discarded unused functions are warned (nor different from 
the case without 'retain')


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/attr-retain.c
  clang/test/CodeGenCXX/attr-retain.cpp
  clang/test/CodeGenCXX/extern-c.cpp
  clang/test/Sema/attr-retain.c

Index: clang/test/Sema/attr-retain.c
===
--- /dev/null
+++ clang/test/Sema/attr-retain.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -Wunused-function
+
+/// We allow 'retain' on non-ELF targets because 'retain' is often used together
+/// with 'used'. 'used' has GC root semantics on macOS and Windows. We want
+/// users to just write retain,used and don't need to dispatch on binary formats.
+
+extern char test1[] __attribute__((retain));   // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+extern const char test2[] __attribute__((retain)); // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+const char test3[] __attribute__((retain)) = "";
+
+struct __attribute__((retain)) s { // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+};
+
+void foo() {
+  static int a __attribute__((retain));
+  int b __attribute__((retain)); // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+  (void)a;
+  (void)b;
+}
+
+__attribute__((retain,used)) static void f0() {}
+__attribute__((retain)) static void f1() {} // expected-warning {{unused function 'f1'}}
+__attribute__((retain)) void f2() {}
+
+/// Test attribute merging.
+int tentative;
+int tentative __attribute__((retain));
+extern int tentative;
+int tentative = 0;
Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,16 +70,19 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_var = internal alias i32, i32* @_ZL12internal_var
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_fn = internal alias i32 (), i32 ()* @_ZL11internal_fnv
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
+  // CHECK: define internal i32 @_ZL11internal_fnv()
 }
 
 namespace PR19411 {
Index: clang/test/CodeGenCXX/attr-retain.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-retain.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -Werror %s -o - | FileCheck %s
+
+struct X0 {
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0C1Ev({{.*}} !retain
+  __attribute__((used, retain)) X0() {}
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0D1Ev({{.*}} !retain
+  __attribute__((used, retain)) ~X0() {}
+};
+
+struct X1 {
+  struct Nested {
+// RETAIN-NOT: 2f0Ev
+// RETAIN: define linkonce_odr{{.*}} @_ZN2X16Nested2f1Ev({{.*}} !retain
+void __attribute__((retain)) f0() {}
+void __attribute__((used, retain)) f1() {}
+  };
+};
+
+// CHECK: define internal void @_ZN10merge_declL4funcEv(){{.*}} !retain
+namespace merge_decl {
+static void func();
+void bar() { void func() __attribute__((used, retain)); }
+static void func() {}
+} // namespace merge_decl
+
+namespace instantiate_member {
+template 
+struct S {
+  void __attribute__((used, retain)) f() {}
+};
+
+void test() {
+  // CHECK: define linkonce_odr{{.*}} void @_ZN18instantiate_member1SIiE1fEv({{.*}} !retain
+  S a;
+}
+} // namespace instantiate_member
Index: clang/test/CodeGen/attr-retain.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-retain.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+/// Set !retain regardless of the target. The backend will lower !retain to
+/// SHF_GNU_RETAIN on ELF and ignore the metadata for other binary formats.
+// CHECK:  @c0 ={{.*}} constant i32 {{.*}} !retain ![[#EMPTY:]]
+// CHECK:  @foo.l0 = internal global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK:  @g0 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g1 ={{.*}} 

[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

MaskRay wrote:
> aaron.ballman wrote:
> > MaskRay wrote:
> > > aaron.ballman wrote:
> > > > Should this be a target-specific attribute as it only has effects on 
> > > > ELF targets?
> > > As I understand it, GCC `retain` is not warned on unsupported targets.
> > > 
> > > Regardless of GCC's choice, I think not having a `warning: unknown 
> > > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes 
> > > sense. `retain` will be usually used together with `used`. In Clang, 
> > > `used` already has "retain" semantics on macOS and Windows (I don't know 
> > > what they do on GCC; GCC folks want orthogonality for ELF and I agree). 
> > > If `retain` is silently ignored on macOS and Windows, then users don't 
> > > need to condition compile for different targets.
> > The other side of that is a user who writes only `retain` and expects it to 
> > do something when it's actually being silently ignored. While they may 
> > usually use it in conjunction with `used`, I'm more worried about the 
> > situation where the user is possibly confused.
> `retain` without `used` can be used on some external linkage definitions, as 
> well as internal linkage definitions which are referenced by live sections. I 
> agree there could be some confusion, but hope with mccall's suggestion 
> (thanks) the documentation is clear.
If `retain` without `used` is an expected usage pattern, then I think we need a 
diagnostic when we ignore the `retain` attribute. I don't think it is 
reasonable to expect users to read the documentation because they won't know 
that they've misused the attribute when we silently ignore it.

Alternatively, would it be possible to make `retain` useful on all targets? 
e.g., when `retain` is used by itself on a declaration compiled for macOS or 
Windows, the backend does whatever it would normally do for `used`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

aaron.ballman wrote:
> MaskRay wrote:
> > aaron.ballman wrote:
> > > Should this be a target-specific attribute as it only has effects on ELF 
> > > targets?
> > As I understand it, GCC `retain` is not warned on unsupported targets.
> > 
> > Regardless of GCC's choice, I think not having a `warning: unknown 
> > attribute 'retain' ignored [-Wunknown-attributes]` diagnostic makes sense. 
> > `retain` will be usually used together with `used`. In Clang, `used` 
> > already has "retain" semantics on macOS and Windows (I don't know what they 
> > do on GCC; GCC folks want orthogonality for ELF and I agree). If `retain` 
> > is silently ignored on macOS and Windows, then users don't need to 
> > condition compile for different targets.
> The other side of that is a user who writes only `retain` and expects it to 
> do something when it's actually being silently ignored. While they may 
> usually use it in conjunction with `used`, I'm more worried about the 
> situation where the user is possibly confused.
`retain` without `used` can be used on some external linkage definitions, as 
well as internal linkage definitions which are referenced by live sections. I 
agree there could be some confusion, but hope with mccall's suggestion (thanks) 
the documentation is clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 325486.
MaskRay marked 2 inline comments as done.
MaskRay added a comment.

incorporate rjmccall's doc suggestion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/attr-retain.c
  clang/test/CodeGenCXX/attr-retain.cpp
  clang/test/CodeGenCXX/extern-c.cpp
  clang/test/Sema/attr-retain.c

Index: clang/test/Sema/attr-retain.c
===
--- /dev/null
+++ clang/test/Sema/attr-retain.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+/// We allow 'retain' on non-ELF targets because 'retain' is often used together
+/// with 'used'. 'used' has GC root semantics on macOS and Windows. We want
+/// users to just write retain,used and don't need to dispatch on binary formats.
+
+extern char test1[] __attribute__((retain));   // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+extern const char test2[] __attribute__((retain)); // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+const char test3[] __attribute__((retain)) = "";
+
+struct __attribute__((retain)) s { // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+};
+
+int g0 __attribute__((retain(0))); // expected-error {{'retain' attribute takes no arguments}}
+
+void foo() {
+  static int a __attribute__((retain));
+  int b __attribute__((retain)); // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+}
+
+__attribute__((used)) static void f0();
+
+/// Test attribute merging.
+int tentative;
+int tentative __attribute__((retain));
+extern int tentative;
+int tentative = 0;
Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,16 +70,19 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_var = internal alias i32, i32* @_ZL12internal_var
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_fn = internal alias i32 (), i32 ()* @_ZL11internal_fnv
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
+  // CHECK: define internal i32 @_ZL11internal_fnv()
 }
 
 namespace PR19411 {
Index: clang/test/CodeGenCXX/attr-retain.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-retain.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -Werror %s -o - | FileCheck %s
+
+struct X0 {
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0C1Ev({{.*}} !retain
+  __attribute__((used, retain)) X0() {}
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0D1Ev({{.*}} !retain
+  __attribute__((used, retain)) ~X0() {}
+};
+
+struct X1 {
+  struct Nested {
+// RETAIN-NOT: 2f0Ev
+// RETAIN: define linkonce_odr{{.*}} @_ZN2X16Nested2f1Ev({{.*}} !retain
+void __attribute__((retain)) f0() {}
+void __attribute__((used, retain)) f1() {}
+  };
+};
+
+// CHECK: define internal void @_ZN10merge_declL4funcEv(){{.*}} !retain
+namespace merge_decl {
+static void func();
+void bar() { void func() __attribute__((used, retain)); }
+static void func() {}
+} // namespace merge_decl
+
+namespace instantiate_member {
+template 
+struct S {
+  void __attribute__((used, retain)) f() {}
+};
+
+void test() {
+  // CHECK: define linkonce_odr{{.*}} void @_ZN18instantiate_member1SIiE1fEv({{.*}} !retain
+  S a;
+}
+} // namespace instantiate_member
Index: clang/test/CodeGen/attr-retain.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-retain.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+/// Set !retain regardless of the target. The backend will lower !retain to
+/// SHF_GNU_RETAIN on ELF and ignore the metadata for other binary formats.
+// CHECK:  @c0 ={{.*}} constant i32 {{.*}} !retain ![[#EMPTY:]]
+// CHECK:  @foo.l0 = internal global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK:  @g0 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g1 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g3 = internal global i32 {{.*}} !retain ![[#EMPTY]]

[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

MaskRay wrote:
> aaron.ballman wrote:
> > Should this be a target-specific attribute as it only has effects on ELF 
> > targets?
> As I understand it, GCC `retain` is not warned on unsupported targets.
> 
> Regardless of GCC's choice, I think not having a `warning: unknown attribute 
> 'retain' ignored [-Wunknown-attributes]` diagnostic makes sense. `retain` 
> will be usually used together with `used`. In Clang, `used` already has 
> "retain" semantics on macOS and Windows (I don't know what they do on GCC; 
> GCC folks want orthogonality for ELF and I agree). If `retain` is silently 
> ignored on macOS and Windows, then users don't need to condition compile for 
> different targets.
The other side of that is a user who writes only `retain` and expects it to do 
something when it's actually being silently ignored. While they may usually use 
it in conjunction with `used`, I'm more worried about the situation where the 
user is possibly confused.



Comment at: clang/lib/Sema/SemaDecl.cpp:2834
   }
+  if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr()) {
+RetainAttr *NewAttr = OldAttr->clone(Context);

MaskRay wrote:
> aaron.ballman wrote:
> > I realize you're following the same pattern as the used attribute, but.. 
> > why is this code even necessary? The attributes are already marked as being 
> > inherited in Attr.td, so I'd not expect to need either of these. (If you 
> > don't know the answer off the top of your head, that's fine -- I can 
> > investigate on my own.)
> Without `setInherited`, `attr-decl-after-definition.c` will fail. (I don't 
> know why..) So I will keep the code but add a test case using the variable 
> `tentative`.
Thanks, if I get the chance, I'll try to look into why this code is necessary 
(and make adjustments if needed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> I have documented the behaviors in clang/include/clang/Basic/AttrDocs.td. Do 
> you have suggestions on the wording?

Included; please let me know what you think.




Comment at: clang/include/clang/Basic/AttrDocs.td:63-76
+The attribute, when attached to a function definition, causes the function to
+be emitted even if it appears that the function is not referenced and can
+otherwise be omitted.
+
+The attribute, when attached to a variable definition with static storage,
+causes the variable to be emitted even if it appears that the variable is not
+referenced.





Comment at: clang/include/clang/Basic/AttrDocs.td:83-91
+This attribute only has effects on ELF targets that support SHF_GNU_RETAIN.
+The attribute, when attached to a function or variable definition, if the
+function or variable is emitted, causes the function or variable to be emitted
+in a unique section with SHF_GNU_RETAIN flag.  This ELF section flag prevents
+garbage collection of the section by supported linkers (GNU ld and gold from
+binutils 2.36 onwards, LLD 13 or newer).
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D96838#2578127 , @rjmccall wrote:

> In D96838#2578101 , @MaskRay wrote:
>
>> In D96838#2578097 , @rjmccall wrote:
>>
>>> In D96838#2578083 , @MaskRay wrote:
>>>
 In D96838#2578073 , @rjmccall 
 wrote:

> GCC 11 hasn't been released yet, so can we still engage with GCC about 
> the semantics of this attribute?  This is a very low-level attribute, and 
> I don't understand why it was made separate instead of just having `used` 
> add the appropriate section flag on targets that support it.

 GCC did overload `used` with the linker garbage collection semantics. Then 
 after discussions (e.g. 
 https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565478.html) they 
 decided to pick the earliest suggestion: add `retain`.

 This makes sense to me: `used` can emit a function which is only used by 
 inline assembly, but the user intention can still be that the whole thing 
 can be GCable.
>>>
>>> There's definitely some merit to distinguishing the cases here — `used` 
>>> doesn't just force the object to be emitted, it also forces the compiler to 
>>> treat it as if there might be accesses to the variable / function that it 
>>> can't see.  Thus, for example, a non-const `used` variable has to be 
>>> assumed to be potentially mutated in arbitrary ways.  I guess my concern is 
>>> that this new attribute still doesn't feel like it covers the use cases 
>>> very well, especially because `used` does have long-established semantics 
>>> of affecting the linker on non-ELF targets.  Basically, to get "full 
>>> strength" semantics that affect the compiler and linker on all targets, you 
>>> have to add a new attribute;
>>
>> Yes.
>>
>>> to get "partial strength" semantics that only affect the compiler, you can 
>>> use the old attribute on ELF, but there's no way to spell it on non-ELF; 
>>> and if you wanted to get "weak" semantics (e.g. to force the symbol to be 
>>> preserved through linking without broadly disabling the optimizer), there's 
>>> no way to spell that anywhere.
>>
>> The semantics can be represented on non-ELF, as long as the binary format 
>> supports multiple sections of the same name.
>
> My concern is less whether the semantics are implementable and more whether 
> there's a way of requesting them in the source language.  `attribute((used))` 
> is expected to prevent link-time stripping on Mach-O and PE/COFF.  We can't 
> just change the long-accepted semantics of the existing attribute after 
> something like 20 years; this is documented behavior.
>
> That said, I can understand why GCC is reluctant to make `attribute((used))` 
> alone start preventing the linker from stripping symbols that it's been able 
> to strip for years.  So I guess really the attribute just has to be 
> understood to have a different meaning on different targets because of this 
> historical divergence.  This seems like something we need to document in the 
> manual.

I have documented the behaviors in `clang/include/clang/Basic/AttrDocs.td`. Do 
you have suggestions on the wording?

> If we wanted to implement the PE/COFF precision improvement, so that `used` 
> only affected one specific declaration, that seems like something we could 
> probably reasonably do, since that behavior is not documented (and may be 
> inconsistent between linkages anyway, if I understand you right).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D96838#2578101 , @MaskRay wrote:

> In D96838#2578097 , @rjmccall wrote:
>
>> In D96838#2578083 , @MaskRay wrote:
>>
>>> In D96838#2578073 , @rjmccall 
>>> wrote:
>>>
 GCC 11 hasn't been released yet, so can we still engage with GCC about the 
 semantics of this attribute?  This is a very low-level attribute, and I 
 don't understand why it was made separate instead of just having `used` 
 add the appropriate section flag on targets that support it.
>>>
>>> GCC did overload `used` with the linker garbage collection semantics. Then 
>>> after discussions (e.g. 
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565478.html) they 
>>> decided to pick the earliest suggestion: add `retain`.
>>>
>>> This makes sense to me: `used` can emit a function which is only used by 
>>> inline assembly, but the user intention can still be that the whole thing 
>>> can be GCable.
>>
>> There's definitely some merit to distinguishing the cases here — `used` 
>> doesn't just force the object to be emitted, it also forces the compiler to 
>> treat it as if there might be accesses to the variable / function that it 
>> can't see.  Thus, for example, a non-const `used` variable has to be assumed 
>> to be potentially mutated in arbitrary ways.  I guess my concern is that 
>> this new attribute still doesn't feel like it covers the use cases very 
>> well, especially because `used` does have long-established semantics of 
>> affecting the linker on non-ELF targets.  Basically, to get "full strength" 
>> semantics that affect the compiler and linker on all targets, you have to 
>> add a new attribute;
>
> Yes.
>
>> to get "partial strength" semantics that only affect the compiler, you can 
>> use the old attribute on ELF, but there's no way to spell it on non-ELF; and 
>> if you wanted to get "weak" semantics (e.g. to force the symbol to be 
>> preserved through linking without broadly disabling the optimizer), there's 
>> no way to spell that anywhere.
>
> The semantics can be represented on non-ELF, as long as the binary format 
> supports multiple sections of the same name.

My concern is less whether the semantics are implementable and more whether 
there's a way of requesting them in the source language.  `attribute((used))` 
is expected to prevent link-time stripping on Mach-O and PE/COFF.  We can't 
just change the long-accepted semantics of the existing attribute after 
something like 20 years; this is documented behavior.

That said, I can understand why GCC is reluctant to make `attribute((used))` 
alone start preventing the linker from stripping symbols that it's been able to 
strip for years.  So I guess really the attribute just has to be understood to 
have a different meaning on different targets because of this historical 
divergence.  This seems like something we need to document in the manual.

If we wanted to implement the PE/COFF precision improvement, so that `used` 
only affected one specific declaration, that seems like something we could 
probably reasonably do, since that behavior is not documented (and may be 
inconsistent between linkages anyway, if I understand you right).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D96838#2578097 , @rjmccall wrote:

> In D96838#2578083 , @MaskRay wrote:
>
>> In D96838#2578073 , @rjmccall wrote:
>>
>>> GCC 11 hasn't been released yet, so can we still engage with GCC about the 
>>> semantics of this attribute?  This is a very low-level attribute, and I 
>>> don't understand why it was made separate instead of just having `used` add 
>>> the appropriate section flag on targets that support it.
>>
>> GCC did overload `used` with the linker garbage collection semantics. Then 
>> after discussions (e.g. 
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565478.html) they 
>> decided to pick the earliest suggestion: add `retain`.
>>
>> This makes sense to me: `used` can emit a function which is only used by 
>> inline assembly, but the user intention can still be that the whole thing 
>> can be GCable.
>
> There's definitely some merit to distinguishing the cases here — `used` 
> doesn't just force the object to be emitted, it also forces the compiler to 
> treat it as if there might be accesses to the variable / function that it 
> can't see.  Thus, for example, a non-const `used` variable has to be assumed 
> to be potentially mutated in arbitrary ways.  I guess my concern is that this 
> new attribute still doesn't feel like it covers the use cases very well, 
> especially because `used` does have long-established semantics of affecting 
> the linker on non-ELF targets.  Basically, to get "full strength" semantics 
> that affect the compiler and linker on all targets, you have to add a new 
> attribute;

Yes.

> to get "partial strength" semantics that only affect the compiler, you can 
> use the old attribute on ELF, but there's no way to spell it on non-ELF; and 
> if you wanted to get "weak" semantics (e.g. to force the symbol to be 
> preserved through linking without broadly disabling the optimizer), there's 
> no way to spell that anywhere.

The semantics can be represented on non-ELF, as long as the binary format 
supports multiple sections of the same name.

Garbage collection is done at section level on both ELF and PE-COFF. On ELF, 
GCC developers decided that a single `used` making the monolithic 
`.data`/`.text` retained loses GC preciseness and is not a good tradeoff. (I 
agree)

So they let the `used` object be placed in a separate section. There may be 
multiple sections of the same name, even in -fno-function-sections 
-fno-data-sections mode.

  .section .text,"ax",@progbits
  ...
  .section .text,"axR",@progbits,unique,1
  ...

This uncovered a Linux kernel problem (I'd say it is that the Linux kernel's 
linker scripts relies on too much on a not-entirely-guaranteed behavior) 
http://gcc.gnu.org/PR99113 
Their later discussion revealed that separation of concerns is good. So they 
re-picked the `retain` idea.

If PE-COFF wants to pick up the idea, it can do that for external linkage 
symbols: it can let `retain` create a separate section with `/INCLUDE:foo` in 
`.drectve`.
For an internal linkage symbol, I think that is likely non-representable in the 
binary format. (maybe rnk can correct me:) )
If PE-COFF decides to do this, perhaps it should revisit whether `used` should 
set `/INCLUDE:foo` as well, as it can nearly match ELF semantics.

For macOS, it can make `retain` pick up the `N_NO_DEAD_STRIP` behavior if it 
wants to match ELF.
Though the current `uses` already retains the subsection.

> Seems like if we're going to the trouble of adding an attribute, that's not 
> the situation we want to end up in.



>> IIUC macOS's always enabled `.subsections_via_symbols` means always 
>> -ffunction-sections/-fdata-sections, so there is no GC preciseness concern.
>
> Yes, the MachO linker will aggressively dead-strip, and `used` prevents that.
>
>> However, the current Windows behavior (`.drectve` include a linker option to 
>> force retaining the whole section) can unnecessarily retain the full 
>> section, in -fno-function-sections or -fno-data-sections mode.
>
> But that Windows behavior is a limitation of the format rather than an intent 
> to provide different semantics on different targets, I'd say.

It is a limitation for internal symbols. But for external symbols it can fully 
match ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D96838#2578083 , @MaskRay wrote:

> In D96838#2578073 , @rjmccall wrote:
>
>> GCC 11 hasn't been released yet, so can we still engage with GCC about the 
>> semantics of this attribute?  This is a very low-level attribute, and I 
>> don't understand why it was made separate instead of just having `used` add 
>> the appropriate section flag on targets that support it.
>
> GCC did overload `used` with the linker garbage collection semantics. Then 
> after discussions (e.g. 
> https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565478.html) they 
> decided to pick the earliest suggestion: add `retain`.
>
> This makes sense to me: `used` can emit a function which is only used by 
> inline assembly, but the user intention can still be that the whole thing can 
> be GCable.

There's definitely some merit to distinguishing the cases here — `used` doesn't 
just force the object to be emitted, it also forces the compiler to treat it as 
if there might be accesses to the variable / function that it can't see.  Thus, 
for example, a non-const `used` variable has to be assumed to be potentially 
mutated in arbitrary ways.  I guess my concern is that this new attribute still 
doesn't feel like it covers the use cases very well, especially because `used` 
does have long-established semantics of affecting the linker on non-ELF 
targets.  Basically, to get "full strength" semantics that affect the compiler 
and linker on all targets, you have to add a new attribute; to get "partial 
strength" semantics that only affect the compiler, you can use the old 
attribute on ELF, but there's no way to spell it on non-ELF; and if you wanted 
to get "weak" semantics (e.g. to force the symbol to be preserved through 
linking without broadly disabling the optimizer), there's no way to spell that 
anywhere.  Seems like if we're going to the trouble of adding an attribute, 
that's not the situation we want to end up in.

> IIUC macOS's always enabled `.subsections_via_symbols` means always 
> -ffunction-sections/-fdata-sections, so there is no GC preciseness concern.

Yes, the MachO linker will aggressively dead-strip, and `used` prevents that.

> However, the current Windows behavior (`.drectve` include a linker option to 
> force retaining the whole section) can unnecessarily retain the full section, 
> in -fno-function-sections or -fno-data-sections mode.

But that Windows behavior is a limitation of the format rather than an intent 
to provide different semantics on different targets, I'd say.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D96838#2578073 , @rjmccall wrote:

> GCC 11 hasn't been released yet, so can we still engage with GCC about the 
> semantics of this attribute?  This is a very low-level attribute, and I don't 
> understand why it was made separate instead of just having `used` add the 
> appropriate section flag on targets that support it.

GCC did overload `used` with the linker garbage collection semantics. Then 
after discussions (e.g. 
https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565478.html) they 
decided to pick the earliest suggestion: add `retain`.

This makes sense to me: `used` can emit a function which is only used by inline 
assembly, but the user intention can still be that the whole thing can be 
GCable.

IIUC macOS's always enabled `.subsections_via_symbols` means always 
-ffunction-sections/-fdata-sections, so there is no GC preciseness concern.
However, the current Windows behavior (`.drectve` include a linker option to 
force retaining the whole section) can unnecessarily retain the full section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

GCC 11 hasn't been released yet, so can we still engage with GCC about the 
semantics of this attribute?  This is a very low-level attribute, and I don't 
understand why it was made separate instead of just having `used` add the 
appropriate section flag on targets that support it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 325336.
MaskRay marked 5 inline comments as done.
MaskRay edited the summary of this revision.
MaskRay added a comment.

Comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/attr-retain.c
  clang/test/CodeGenCXX/attr-retain.cpp
  clang/test/CodeGenCXX/extern-c.cpp
  clang/test/Sema/attr-retain.c

Index: clang/test/Sema/attr-retain.c
===
--- /dev/null
+++ clang/test/Sema/attr-retain.c
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+/// We allow 'retain' on non-ELF targets because 'retain' is often used together
+/// with 'used'. 'used' has GC root semantics on macOS and Windows. We want
+/// users to just write retain,used and don't need to dispatch on binary formats.
+
+extern char test1[] __attribute__((retain));   // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+extern const char test2[] __attribute__((retain)); // expected-warning {{'retain' attribute ignored on a non-definition declaration}}
+const char test3[] __attribute__((retain)) = "";
+
+struct __attribute__((retain)) s { // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+};
+
+int g0 __attribute__((retain(0))); // expected-error {{'retain' attribute takes no arguments}}
+
+void foo() {
+  static int a __attribute__((retain));
+  int b __attribute__((retain)); // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+}
+
+__attribute__((used)) static void f0();
+
+/// Test attribute merging.
+int tentative;
+int tentative __attribute__((retain));
+extern int tentative;
+int tentative = 0;
Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -70,16 +70,19 @@
 __attribute__((used)) static int duplicate_internal_fn() { return 0; }
   }
 
-  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn 
+  // CHECK: @llvm.used = appending global {{.*}} @internal_var {{.*}} @internal_fn
 
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_var = internal alias i32, i32* @_ZL12internal_var
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_fn = internal alias i32 (), i32 ()* @_ZL11internal_fnv
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
+  // CHECK: define internal i32 @_ZL11internal_fnv()
 }
 
 namespace PR19411 {
Index: clang/test/CodeGenCXX/attr-retain.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-retain.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple -Werror %s -o - | FileCheck %s
+
+struct X0 {
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0C1Ev({{.*}} !retain
+  __attribute__((used, retain)) X0() {}
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0D1Ev({{.*}} !retain
+  __attribute__((used, retain)) ~X0() {}
+};
+
+struct X1 {
+  struct Nested {
+// RETAIN-NOT: 2f0Ev
+// RETAIN: define linkonce_odr{{.*}} @_ZN2X16Nested2f1Ev({{.*}} !retain
+void __attribute__((retain)) f0() {}
+void __attribute__((used, retain)) f1() {}
+  };
+};
+
+// CHECK: define internal void @_ZN10merge_declL4funcEv(){{.*}} !retain
+namespace merge_decl {
+static void func();
+void bar() { void func() __attribute__((used, retain)); }
+static void func() {}
+} // namespace merge_decl
+
+namespace instantiate_member {
+template 
+struct S {
+  void __attribute__((used, retain)) f() {}
+};
+
+void test() {
+  // CHECK: define linkonce_odr{{.*}} void @_ZN18instantiate_member1SIiE1fEv({{.*}} !retain
+  S a;
+}
+} // namespace instantiate_member
Index: clang/test/CodeGen/attr-retain.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-retain.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+/// Set !retain regardless of the target. The backend will lower !retain to
+/// SHF_GNU_RETAIN on ELF and ignore the metadata for other binary formats.
+// CHECK:  @c0 ={{.*}} constant i32 {{.*}} !retain ![[#EMPTY:]]
+// CHECK:  @foo.l0 = internal global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK:  @g0 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g1 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g3 = internal global i32 {{.*}} 

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

aaron.ballman wrote:
> Should this be a target-specific attribute as it only has effects on ELF 
> targets?
As I understand it, GCC `retain` is not warned on unsupported targets.

Regardless of GCC's choice, I think not having a `warning: unknown attribute 
'retain' ignored [-Wunknown-attributes]` diagnostic makes sense. `retain` will 
be usually used together with `used`. In Clang, `used` already has "retain" 
semantics on macOS and Windows (I don't know what they do on GCC; GCC folks 
want orthogonality for ELF and I agree). If `retain` is silently ignored on 
macOS and Windows, then users don't need to condition compile for different 
targets.



Comment at: clang/lib/Sema/SemaDecl.cpp:2834
   }
+  if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr()) {
+RetainAttr *NewAttr = OldAttr->clone(Context);

aaron.ballman wrote:
> I realize you're following the same pattern as the used attribute, but.. why 
> is this code even necessary? The attributes are already marked as being 
> inherited in Attr.td, so I'd not expect to need either of these. (If you 
> don't know the answer off the top of your head, that's fine -- I can 
> investigate on my own.)
Without `setInherited`, `attr-decl-after-definition.c` will fail. (I don't know 
why..) So I will keep the code but add a test case using the variable 
`tentative`.



Comment at: clang/test/CodeGen/attr-retain.c:21
+int g1 __attribute__((retain));
+__attribute__((retain)) static int g2;
+__attribute__((used,retain)) static int g3;

phosek wrote:
> Would it be possible to also include negative check for `g2`?
```
// CHECK-NEXT: @g1 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
// CHECK-NEXT: @g3 = internal global i32 {{.*}} !retain ![[#EMPTY]]
```

excludes accidental g2.



Comment at: clang/test/Sema/attr-retain.c:3
+
+extern char test1[] __attribute__((retain)); // expected-warning {{'retain' 
attribute ignored}}
+extern const char test2[] __attribute__((retain)); // expected-warning 
{{'retain' attribute ignored}}

aaron.ballman wrote:
> Oof, this is not a particularly friendly diagnostic (there's no mention of 
> *why* the attribute is ignored, so it's hard to know how to respond to the 
> diagnostic as a user).
I created the test based on the existing attr-used.c. Let me figure out how to 
make the diagnostic friendlier...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2655
+
+def Retain : InheritableAttr {
+  let Spellings = [GCC<"retain">];

Should this be a target-specific attribute as it only has effects on ELF 
targets?



Comment at: clang/lib/Sema/SemaDecl.cpp:2834
   }
+  if (RetainAttr *OldAttr = Old->getMostRecentDecl()->getAttr()) {
+RetainAttr *NewAttr = OldAttr->clone(Context);

I realize you're following the same pattern as the used attribute, but.. why is 
this code even necessary? The attributes are already marked as being inherited 
in Attr.td, so I'd not expect to need either of these. (If you don't know the 
answer off the top of your head, that's fine -- I can investigate on my own.)



Comment at: clang/test/Sema/attr-retain.c:3
+
+extern char test1[] __attribute__((retain)); // expected-warning {{'retain' 
attribute ignored}}
+extern const char test2[] __attribute__((retain)); // expected-warning 
{{'retain' attribute ignored}}

Oof, this is not a particularly friendly diagnostic (there's no mention of 
*why* the attribute is ignored, so it's hard to know how to respond to the 
diagnostic as a user).



Comment at: clang/test/Sema/attr-retain.c:8
+void foo() {
+  int l __attribute__((retain)); // expected-warning {{'retain' attribute only 
applies to variables with non-local storage, functions, and Objective-C 
methods}}
+}

Can you also add a test case showing the attribute does not accept any 
arguments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Petr Hosek via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGen/attr-retain.c:11
+/// Set !retain only on ELF platforms.
+// NORETAIN-NOT: !retain
+

There are no `FileCheck --check-prefixes=NORETAIN` invocations so this is 
unused.



Comment at: clang/test/CodeGen/attr-retain.c:21
+int g1 __attribute__((retain));
+__attribute__((retain)) static int g2;
+__attribute__((used,retain)) static int g3;

Would it be possible to also include negative check for `g2`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

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

[PATCH] D96838: Add GNU attribute 'retain'

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 325233.
MaskRay retitled this revision from "CodeGen: Set !retain metadata on UsedAttr 
definitions for FreeBSD/Fuchsia/Linux" to "Add GNU attribute 'retain'".
MaskRay edited the summary of this revision.
MaskRay added a comment.
Herald added a reviewer: aaron.ballman.

Add 'retain' instead (latest GCC resolution)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96838

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/attr-retain.c
  clang/test/CodeGenCXX/attr-retain.cpp
  clang/test/CodeGenCXX/extern-c.cpp
  clang/test/Sema/attr-retain.c

Index: clang/test/Sema/attr-retain.c
===
--- /dev/null
+++ clang/test/Sema/attr-retain.c
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+extern char test1[] __attribute__((retain)); // expected-warning {{'retain' attribute ignored}}
+extern const char test2[] __attribute__((retain)); // expected-warning {{'retain' attribute ignored}}
+const char test3[] __attribute__((retain)) = "";
+
+void foo() {
+  int l __attribute__((retain)); // expected-warning {{'retain' attribute only applies to variables with non-local storage, functions, and Objective-C methods}}
+}
Index: clang/test/CodeGenCXX/extern-c.cpp
===
--- clang/test/CodeGenCXX/extern-c.cpp
+++ clang/test/CodeGenCXX/extern-c.cpp
@@ -75,11 +75,14 @@
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_var = internal alias i32, i32* @_ZL12internal_var
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
   // CHECK: @internal_fn = internal alias i32 (), i32 ()* @_ZL11internal_fnv
+  // CHECK-NOT: !retain
   // CHECK-NOT: @unused
   // CHECK-NOT: @duplicate_internal
+  // CHECK: define internal i32 @_ZL11internal_fnv()
 }
 
 namespace PR19411 {
Index: clang/test/CodeGenCXX/attr-retain.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-retain.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple %s -o - | FileCheck %s
+
+struct X0 {
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0C1Ev({{.*}} !retain
+  __attribute__((used,retain)) X0() {}
+  // RETAIN: define linkonce_odr{{.*}} @_ZN2X0D1Ev({{.*}} !retain
+  __attribute__((used,retain)) ~X0() {}
+};
+
+struct X1 {
+  struct Nested {
+// RETAIN-NOT: 2f0Ev
+// RETAIN: define linkonce_odr{{.*}} @_ZN2X16Nested2f1Ev({{.*}} !retain
+void __attribute__((retain)) f0() {}
+void __attribute__((used,retain)) f1() {}
+  };
+};
+
+// CHECK: define internal void @_ZN10merge_declL4funcEv(){{.*}} !retain
+namespace merge_decl {
+static void func();
+void bar() { void func() __attribute__((used,retain)); }
+static void func() {}
+}
+
+namespace instantiate_member {
+template 
+struct S {
+  void __attribute__((used,retain)) f() {}
+};
+
+void test() {
+  // CHECK: define linkonce_odr{{.*}} void @_ZN18instantiate_member1SIiE1fEv({{.*}} !retain
+  S a;
+}
+}
Index: clang/test/CodeGen/attr-retain.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-retain.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+// CHECK:  @c0 ={{.*}} constant i32 {{.*}} !retain ![[#EMPTY:]]
+// CHECK:  @foo.l0 = internal global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK:  @g0 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g1 ={{.*}} global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g3 = internal global i32 {{.*}} !retain ![[#EMPTY]]
+// CHECK-NEXT: @g4 = internal global i32 0, section ".data.g"{{.*}} !retain ![[#EMPTY]]
+
+/// Set !retain only on ELF platforms.
+// NORETAIN-NOT: !retain
+
+const int c0 __attribute__((retain)) = 42;
+
+void foo() {
+  static int l0 __attribute__((retain)) = 2;
+}
+
+__attribute__((retain)) int g0;
+int g1 __attribute__((retain));
+__attribute__((retain)) static int g2;
+__attribute__((used,retain)) static int g3;
+__attribute__((used,retain,section(".data.g"))) static int g4;
+
+// CHECK:  define dso_local void @f0(){{.*}} !retain ![[#EMPTY]]
+// CHECK-NOT:  @f1
+// CHECK:  define internal void @f2(){{.*}} !retain ![[#EMPTY]]
+
+void __attribute__((retain)) f0(void) {}
+static void __attribute__((retain)) f1(void) {}
+static void __attribute__((used,retain)) f2(void) {}
+
+// CHECK:  [[#EMPTY]] = !{}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -2831,6 +2831,11 @@
 NewAttr->setInherited(true);