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 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 the
> S_ATTR_NO_DEAD_STRIP symbol attribute for `!retain`.
>
> > If so, then I don't think we need to make it target-specific or otherwise
> > diagnose it.
>
> Thanks! Marks as resolved.
Thank you for your patience while I figured this out. :-)
================
Comment at: clang/include/clang/Basic/AttrDocs.td:63
+ let Content = [{
+This attribute, when attached to a function or variable definition, indicates
+that there may be references to the entity which are not apparent in the source
----------------
================
Comment at: clang/include/clang/Basic/AttrDocs.td:85
+ let Content = [{
+This attribute, when attached to a function or variable definition, prevents
+the linker from discarding the definition. If the compiler does not emit the
----------------
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
