[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