vsavchenko added a comment. In D93630#2479757 <https://reviews.llvm.org/D93630#2479757>, @aaron.ballman wrote:
> In D93630#2479343 <https://reviews.llvm.org/D93630#2479343>, @vsavchenko > wrote: > >> In D93630#2479260 <https://reviews.llvm.org/D93630#2479260>, @aaron.ballman >> wrote: >> >>> 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? >> >> We do not decide whether it is declaration or statement. I do get that it >> is a matter of perspective, but right now I prefer to read it like this: >> **BEFORE**: if we've seen a GNU-style attribute parse whatever comes next as >> a declaration >> **AFTER**: if we've seen a GNU-style attribute except for the case when all >> of the parsed attributes are known to be statement attributes, parse >> whatever comes next as a declaration >> >> So, essentially, when we see statement attributes only, >> attribute/declaration heuristic is canceled and we parse exactly the way we >> would've parsed as if no attributes present. > > I think this is a defensible design, but it still has the potential to change > the parsing behavior in some circumstances and I want to make sure those > circumstances are ones we know about. Note, I still think we should > coordinate this change with the GCC folks. GCC has a lot of attributes that > Clang does not have, so my concern with GCC is that Clang supports writing > attributes in this way and GCC can't support it for some reason and it causes > issues for an attribute we want to share between implementations. I don't > have a specific case in mind that may cause an issue but they have a lot of > attributes I'm unfamiliar with. This is where we also pretty defensive and conservative, if we see any unknown attributes, we stick with the "attribute -> declaration" rule. >>> 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. >> >> In both situations, the code contains errors. I don't think that this is a >> valid argument, to be honest. Actually, the nature of this change is that >> it starts to parse strictly MORE cases. > > My concerns are if there are cases where we would start to reject valid code > based on the presence of a GNU statement attribute or where we'd regress > diagnostics. This seemed like a case where we were regressing diagnostics and > could potentially reject otherwise valid code (using a different attribute > that wouldn't generate an error in the way `fallthrough` does). We previously > were saying "aha, this is a declaration and the attribute cannot apply to it" > which is strictly better than "aha, I have no idea what this identifier is > doing here". However, I'm changing my opinion here -- see below. > >>> 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;` >> >> This is **not** a valid code if you remove `__attribute__((suppress))`. `i = >> 12` without a prior declaration of `i` will cause the `use of undeclared >> identifier 'i'` error. You can try it with any compiler: >> https://godbolt.org/z/P43nxn. > > Thank you for pointing out that my test case was bad in the first place -- I > forgot that *block scope* variables cannot be implicit `int`, but *file > scope* ones can (in C, not in C++): https://godbolt.org/z/7TjGT3 > > When I went back and re-ran my test with the variable at file scope, the > behavior stayed the same with and without the patch: > > F:\llvm-project>cat "F:\Aaron Ballman\Desktop\test.c" > __attribute__((fallthrough)) i = 12; > j = 12; > > Before patch: > > F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -std=c89 > -fsyntax-only "F:\Aaron Ballman\Desktop\test.c" > F:\Aaron Ballman\Desktop\test.c:1:30: warning: declaration specifier > missing, defaulting to 'int' > __attribute__((fallthrough)) i = 12; > ^ > int > F:\Aaron Ballman\Desktop\test.c:1:16: error: 'fallthrough' attribute cannot > be applied to a declaration > __attribute__((fallthrough)) i = 12; > ^ ~ > F:\Aaron Ballman\Desktop\test.c:2:1: warning: declaration specifier missing, > defaulting to 'int' > j = 12; > ^ > int > 2 warnings and 1 error generated. > > After patch: > > F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -std=c89 > -fsyntax-only "F:\Aaron Ballman\Desktop\test.c" > F:\Aaron Ballman\Desktop\test.c:1:30: warning: declaration specifier > missing, defaulting to 'int' > __attribute__((fallthrough)) i = 12; > ^ > int > F:\Aaron Ballman\Desktop\test.c:1:16: error: 'fallthrough' attribute cannot > be applied to a declaration > __attribute__((fallthrough)) i = 12; > ^ ~ > F:\Aaron Ballman\Desktop\test.c:2:1: warning: declaration specifier > missing, defaulting to 'int' > j = 12; > ^ > int > 2 warnings and 1 error generated. > > Huttah! Yay! >> I honestly don't see a scenario when the user has some piece of code that is >> parsed as a statement, then they mindfully add a //statement// attribute, >> and expect it to be parsed as a declaration after that. > > 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. > 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! >>> 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! > 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! 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
