[PATCH] D93630: [Attr] Apply GNU-style attributes to expression statements

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D93630#2470048 <https://reviews.llvm.org/D93630#2470048>, @vsavchenko wrote:

> In D93630#2468853 <https://reviews.llvm.org/D93630#2468853>, @aaron.ballman wrote:
>
>> Yeah, I kind of figured that might be the cause. I'm not 100% convinced (one way or the other) if the suppress attribute should get a GNU spelling. The `[[]]` spellings are available in all language modes (we have `-fdouble-square-bracket-attributes` to enable this) and don't run afoul of the "guess what this attribute appertains to" problem that GNU-style attributes do.
>
> I don't think that we can force our users into adding this flag.

You don't have to -- ObjC could always imply the flag is set (and I imagine ObjC will eventually update to C23 as the base language, where it'll be supported anyway).

> Also, Obj-C codebases already use a good amount of GNU-style attributes, so it is pretty natural there.

IIRC, those attributes are often hidden behind macros, so I'm not certain how critical this is if the `-fdouble-square-bracket-attributes` feature was enabled in ObjC mode.



================
Comment at: clang/lib/Parse/ParseStmt.cpp:213
              ParsedStmtContext()) &&
-        (GNUAttributeLoc.isValid() || isDeclarationStatement())) {
+        ((GNUAttributeLoc.isValid() && !Attrs.back().isStmtAttr()) ||
+         isDeclarationStatement())) {
----------------
vsavchenko wrote:
> aaron.ballman wrote:
> > I think you need to ensure there's at least one attribute before checking `!Attrs.back().isStmtAttr()` as this may cause problems if the user does something odd like `__attribute__(()) int x;` (I don't know if this will result in a valid `GNUAttributeLoc` with no attributes or not.)
> > 
> > I'm not certain that logic is perfect either. It would be pretty mysterious to handle these cases differently:
> > ```
> > __attribute__((stmt_attr, decl_attr)) int a, b, c;
> > __attribute__((decl_attr, stmt_attr)) int x, y, z;
> > ```
> Yep, my bad.  I changed it so that ALL the attributes in the front should be statement attributes.
I think this is a better approach but it still doesn't leave us a clean way to handle attributes that are both declaration and statement attributes. I'm nervous about this in the same way I'm nervous about GNU attributes sliding around in general -- it doesn't always work out the way a user might expect.

`nomerge` is a good example. You currently cannot write this code:
```
int f(void);

void func(void) {
  __attribute__((nomerge)) static int i = f();
}
```
because of the way GNU attributes start a declaration. However, I think this could also be considered a bug because `nomerge` on a statement (which this also is) will ensure that the call expression to `f()` is not merged during optimization.

If we now say "GNU attribute can now be used at the start of an arbitrary statement", we have to late parse the attribute until we know what kind of statement we have. Because the user could have written:
```
void func(void) {
  __attribute__((nomerge)) extern void f(void), g(int), h(void);
}
```
which is currently accepted today and would change meaning if we treated `nomerge` as a statement attribute.


================
Comment at: clang/test/Parser/stmt-attributes.c:89
+
+__attribute__((nomerge)) static int i; // expected-error {{'nomerge' attribute cannot be applied to a declaration}}
----------------
This is a semi-good example of the kind of behavior I was worried about -- `nomerge` can be applied to a declaration or a statement, just not *this* kind of declaration, and so the diagnostic is confusing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93630/new/

https://reviews.llvm.org/D93630