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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> I guess I want to clarify one point here, after this patch the parser **will not assume** that statement always follows statement attributes.  We simply turn off the assumption that what follows is a declaration, parser will simply determine whether it is a statement or a declaration, it can do it on its own without attribute's "help".

We used to say that if there's a GNU attribute (or we're parsing a declaration statement), what follows must be a declaration. Now we're using the attributes in the given attribute list to decide whether what follows is a declaration or a statement (if all of the attributes are statement attributes, we don't treat what follows as being a declaration unless we're in a declaration statement). Or am I misreading the code?

e.g.,

  void foo(void) {
    __attribute__((fallthrough)) i = 12;
  }

Before patch:

  F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron Ballman\Desktop\test.c"
  F:\Aaron Ballman\Desktop\test.c:2:32: warning: type specifier missing, defaults to 'int' [-Wimplicit-int]
    __attribute__((fallthrough)) i = 12;
                                 ^
  F:\Aaron Ballman\Desktop\test.c:2:18: error: 'fallthrough' attribute cannot be applied to a declaration
    __attribute__((fallthrough)) i = 12;
                   ^             ~
  1 warning and 1 error generated.

After patch:

  F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron Ballman\Desktop\test.c"
  F:\Aaron Ballman\Desktop\test.c:2:32: error: use of undeclared identifier 'i'
    __attribute__((fallthrough)) i = 12;
                                 ^
  1 error generated.

So I think this will cause issues for the suppression attribute you're working on if we allow it to have a GNU spelling with valid C89 code like; `__attribute__((suppress)) i = 12;` or `__attribute__((suppress)) g() { return 12; }` which may be intended to suppress the "type specifier missing" diagnostic (somewhat amusingly, since the user could also just add the `int` to the declaration since they're changing the code anyway).



================
Comment at: clang/lib/Parse/ParseStmt.cpp:213
              ParsedStmtContext()) &&
-        (GNUAttributeLoc.isValid() || isDeclarationStatement())) {
+        ((GNUAttributeLoc.isValid() && !Attrs.back().isStmtAttr()) ||
+         isDeclarationStatement())) {
----------------
vsavchenko wrote:
> aaron.ballman wrote:
> > 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.
> > 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 look at `clang/test/Sema/attr-nomerge.cpp`, we can see that we have exactly the same behavior as in C++ with `[[clang::nomerge]]`:
> 
> ```
> [[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute cannot be applied to a declaration}}
> ```
> 
> > 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.
> 
> I checked that as well and for this example before this patch, after this patch, and in C++ when using `[[clang::nomerge]]` we get exactly the same behavior: 3 errors `'nomerge' attribute cannot be applied to a declaration`.
> 
> 
> If we look at clang/test/Sema/attr-nomerge.cpp, we can see that we have exactly the same behavior as in C++ with [[clang::nomerge]]

I don't read it quite the same way -- in C and C++, the syntactic position of the attribute defines what the attribute appertains to, unlike with GNU attributes. So the end result is the same, but for different reasons. In the case of the C or C++ attribute spelling, an attribute at that position applies to all of the declarations in the declaration group, and the declaration is of the wrong type (hence the error). In the case of the GNU attribute, we guess wrong whether the attribute appertains to the declaration(s) or the statement because we don't know which to pick solely based on the name of the attribute. We wind up picking the declaration version and it applies to the wrong kind of declaration so we get the error. However, it's defensible to argue that as part of the heuristic for determining what the attribute appertains to we should be checking that the attribute actually applies successfully -- e.g., we try it as a decl attr, find that it doesn't apply, then try again as a stmt attr. (Note, I'm not at all suggesting that we should *do* this work, just pointing out that the "sliding" behavior of GNU attributes makes this tricky to say what the right answer is.)

> I checked that as well and for this example before this patch, after this patch, and in C++ when using [[clang::nomerge]] we get exactly the same behavior: 3 errors 'nomerge' attribute cannot be applied to a declaration.

Huh, that's odd, because for me, before this patch, I get no errors with that example: https://godbolt.org/z/fsGr5n


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93630