vsavchenko added inline comments.

================
Comment at: clang/lib/Parse/ParseStmt.cpp:213
              ParsedStmtContext()) &&
-        (GNUAttributeLoc.isValid() || isDeclarationStatement())) {
+        ((GNUAttributeLoc.isValid() && !Attrs.back().isStmtAttr()) ||
+         isDeclarationStatement())) {
----------------
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`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93630

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to