[clang] Force AttributedStmtClass to not be scope parents #125370
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Yutong Zhu (YutongZhuu) ChangesThis PR addresses #84072. Full diff: https://github.com/llvm/llvm-project/pull/125370.diff 1 Files Affected:
diff --git a/clang/lib/Sema/JumpDiagnostics.cpp b/clang/lib/Sema/JumpDiagnostics.cpp
index d465599450e7ff..c5577e09a86a1c 100644
--- a/clang/lib/Sema/JumpDiagnostics.cpp
+++ b/clang/lib/Sema/JumpDiagnostics.cpp
@@ -597,15 +597,6 @@ void JumpScopeChecker::BuildScopeInformation(Stmt *S,
LabelAndGotoScopes[S] = ParentScope;
break;
- case Stmt::AttributedStmtClass: {
- AttributedStmt *AS = cast<AttributedStmt>(S);
- if (GetMustTailAttr(AS)) {
- LabelAndGotoScopes[AS] = ParentScope;
- MustTailStmts.push_back(AS);
- }
- break;
- }
-
case Stmt::OpenACCComputeConstructClass: {
unsigned NewParentScope = Scopes.size();
OpenACCComputeConstruct *CC = cast<OpenACCComputeConstruct>(S);
@@ -658,7 +649,13 @@ void JumpScopeChecker::BuildScopeInformation(Stmt *S,
Next = SC->getSubStmt();
else if (LabelStmt *LS = dyn_cast<LabelStmt>(SubStmt))
Next = LS->getSubStmt();
- else
+ else if (AttributedStmt *AS = dyn_cast<AttributedStmt>(SubStmt)) {
+ if (GetMustTailAttr(AS)) {
+ LabelAndGotoScopes[AS] = ParentScope;
+ MustTailStmts.push_back(AS);
+ }
+ Next = AS->getSubStmt();
+ } else
break;
LabelAndGotoScopes[SubStmt] = ParentScope;
|
|
Could you add a test case - check in clang/test to see if other tests for the diagnostic text in the original bug, and add a test case for that nearby (maybe the same file the diagnostic is already tested in)? |
|
Thank you for the fix!
Also, please add a more descriptive summary of what's changing and why (it makes it easier on us when we have to dig through a blame a few years down the line) and a release note in |
Do you mean I should check if there exists a test for the original bug, if not, add one? |
I think David means to find a related test file to add a new test case into (and you can tell the file is related by searching for tests that have "cannot jump from switch statement to this case label" and adding the new test there, such as |
YutongZhuu
force-pushed
the
dev_issue_84072
branch
from
e71018f to
380ae20
Compare
Done |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Aside from the issue with the release notes, I think this LGTM!
clang/docs/ReleaseNotes.rst
Outdated
There was a problem hiding this comment.
It looks like a bunch of unrelated release notes came in; these should be dropped.
There was a problem hiding this comment.
I made this change to resolve the conflicts with main. Should I not do this?
There was a problem hiding this comment.
Resolving the conflict with main should have left just the one-line addition from your PR.
There was a problem hiding this comment.
I see, thanks for the explanations.
clang/docs/ReleaseNotes.rst
Outdated
There was a problem hiding this comment.
| - Clang now forces attributes to not be scope parents (#84072). | |
| - A statement attribute applied to a ``case`` label no longer suppresses | |
| 'bypassing variable initialization' diagnostics (#84072). |
YutongZhuu
force-pushed
the
dev_issue_84072
branch
from
f654f53 to
1923379
Compare
YutongZhuu
force-pushed
the
dev_issue_84072
branch
from
1923379 to
fb756c0
Compare
Addressed, thanks for reviewing :) |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
LGTM, thank you for the fix!
|
@YutongZhuu Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/15573 Here is the relevant piece of the build log for the reference |
Example from the issue:
```c++
void Func(int x) {
switch (x) {
[[likely]] case 0:
case 1:
int i = 3;
case 2:
break;
}
}
```
Clang checks if ``case 2`` can be reachable by checking its scope. The
variable declaration should create a scope containing ``case 2``, but
due to the structure of the AST, ``case 2`` gets the scope of the
``likely`` statement, but not ``int i = 3;``. Therefore, I changed this
code to force attribute statement not to be scope parents.
Fixes llvm#84072
Example from the issue:
```c++
void Func(int x) {
switch (x) {
[[likely]] case 0:
case 1:
int i = 3;
case 2:
break;
}
}
```
Clang checks if ``case 2`` can be reachable by checking its scope. The
variable declaration should create a scope containing ``case 2``, but
due to the structure of the AST, ``case 2`` gets the scope of the
``likely`` statement, but not ``int i = 3;``. Therefore, I changed this
code to force attribute statement not to be scope parents.
Fixes llvm#84072
Example from the issue:
```c++
void Func(int x) {
switch (x) {
[[likely]] case 0:
case 1:
int i = 3;
case 2:
break;
}
}
```
Clang checks if ``case 2`` can be reachable by checking its scope. The
variable declaration should create a scope containing ``case 2``, but
due to the structure of the AST, ``case 2`` gets the scope of the
``likely`` statement, but not ``int i = 3;``. Therefore, I changed this
code to force attribute statement not to be scope parents.
Fixes llvm#84072
5 participants
This PR addresses #84072.
Example from the issue:
Clang checks if
case 2can be reachable by checking its scope. The variable declaration should create a scope containingcase 2, but due to the structure of the AST,case 2gets the scope of thelikelystatement, but notint i = 3;. Therefore, I changed this code to force attribute statement not to be scope parents. Please confirm this is something legal to do :)