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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> In D93630#2479757 <https://reviews.llvm.org/D93630#2479757>, @aaron.ballman wrote:
>
>> There are attributes like `nomerge` which are both a declaration and a statement attribute, and there are other attributes like `suppress` that are statement attributes which can reasonably be written on a declaration statement. So the scenario I am worried about is when the code is being parsed as a declaration, the user adds one of these attributes and the decision changes to parse as a statement rather than a declaration. I'm not yet convinced we can't run into that situation with this change, but the implicit `int` thing was the only major edge case I could come up with and I've alllllmost convinced myself we can't run into an issue with it. I think our parser predicate test (on line 218 of your patch) for whether something is a declaration statement or not is what's helping avoid most of the parsing concerns I have because they usually will find some declaration specifier to help get the right answer.
>
> I actually thought about this.  Essentially there are 3 interesting cases here:
>
> 1. the user has a declaration that is parsed as a declaration only because of the attribute and wants to add a statement attribute
>
>   __attribute__((X)) i = 12;
>   // into
>   __attribute__((X, fallthrough)) i = 12;
>
> We don't want to break working code, so we maintain the rule and parse it as a declaration.
>
> 2. the user has a statement and wants to add a statement attribute
>
>   i = 12; // assignment
>   // into
>   __attribute__((stmt_attr)) i = 12; // still assignment
>
> We don't use the rule and don't break the code, that's why we do it!
>
> 3. the user has a declaration-or-statement attribute on a declaration that is parsed as declaration only because of this attribute
>
>   __attribute__((both-decl-and-stmt)) i = 12; // implicit int declaration
>   // after the Clang update
>   __attribute__((both-decl-and-stmt)) i = 12; // error!
>
> That is a regression, but it looks like statement attributes are pretty rare, let alone declaration-or-statement attributes in C.  Also, it might cause the problem only when the user relied on the "attribute -> declaration" rule.  So, it seems pretty low chance, and we get a bit more consistency in what "statement attribute" means.

I agree with these three cases. I did come up with a fourth case: the user wants to attribute something that is a statement which declares things but is not a declaration statement. e.g., `__attribute__((whatever)) [](){}();` (or similar using blocks instead of lambdas). We don't parse this construct on trunk (https://godbolt.org/z/3e83of) and I tried it with your patch applied and it still wouldn't parse, so it may make for a good test case to add.

>> FWIW, while testing various situations out, I think I found a Clang bug where we reject valid code (https://godbolt.org/z/ooqMvP) that *might* cause us problems if we apply this patch and ever fix that rejects valid bug. We should not be rejecting any of these functions, but we fail to note the implicit `int` with the declaration within `foo()` (note how the presence of the `extern` declaration specifier in `quux()` fixes things).
>
> Yikes, that's pretty bad!

I filed https://bugs.llvm.org/show_bug.cgi?id=48672 for it.

>>>> 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).
>>>
>>> Again, we **do not force** the parser to parse whatever follows the statement attribute as a statement.  `__attribute__((suppress))` will not change how we parse things here.
>>
>> I agree that it shouldn't (in theory), but I've not been able to convince myself that it doesn't (in practice). I'm getting there though, and I appreciate your patience while we discuss the concerns.
>
> I actually want to thank you instead.  It is a pretty tricky situation here with all this, so thank you for going so patiently from one case to another!

Huttah for quality code review practices! :-)

>> Concretely, I think we could use some more C test coverage for the various places where implicit `int` can appear in a declaration. Here are the places I could come up with off the top of my head: https://godbolt.org/z/6h3MTr
>
> That's awesome, I'll incorporate that into the patch!

I'm going to see if I can run the patch against our internal corpus here at work to see if it shakes out any breakages to real world code. I've not tried this before, so I have no idea how long it will take before I get results back, but I'll let you know when I have them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93630