[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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 se

[PATCH] D96838: Add GNU attribute 'retain'

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)

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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 a

[PATCH] D96838: Add GNU attribute 'retain'

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 u

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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 i

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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 w

[PATCH] D96838: Add GNU attribute 'retain'

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: > > > MaskR

[PATCH] D96838: Add GNU attribute 'retain'

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 wrot

[PATCH] D96838: Add GNU attribute 'retain'

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 w

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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://revi

[PATCH] D96838: Add GNU attribute 'retain'

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 b

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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:

[PATCH] D96838: Add GNU attribute 'retain'

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 effec

[PATCH] D96838: Add GNU attribute 'retain'

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 t

[PATCH] D96838: Add GNU attribute 'retain'

MaskRay added a comment. In D96838#2578127 , @rjmccall wrote: > In D96838#2578101 , @MaskRay wrote: > >> In D96838#2578097 , @rjmccall wrote: >> >>> In D96838#2578083

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. In D96838#2578101 , @MaskRay wrote: > In D96838#2578097 , @rjmccall wrote: > >> In D96838#2578083 , @MaskRay wrote: >> >>> In D96838#2578073

[PATCH] D96838: Add GNU attribute 'retain'

MaskRay added a comment. In D96838#2578097 , @rjmccall wrote: > In D96838#2578083 , @MaskRay wrote: > >> In D96838#2578073 , @rjmccall wrote: >> >>> GCC 11 hasn't been releas

[PATCH] D96838: Add GNU attribute 'retain'

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?

[PATCH] D96838: Add GNU attribute 'retain'

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 separa

[PATCH] D96838: Add GNU attribute 'retain'

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 s

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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? Com

[PATCH] D96838: Add GNU attribute 'retain'

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

[PATCH] D96838: Add GNU attribute 'retain'

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.