[PATCH] D96838: Add GNU attribute 'retain'

Mon Feb 22 10:46:11 PST 2021

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