[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>()) {
+    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