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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
