[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 |
clang/docs/ReleaseNotes.rst
Outdated
|
|
||
| - Clang now omits shadow warnings for enum constants in separate class scopes (#GH62588). | ||
|
|
||
| - When diagnosing an unused return value of a type declared ``[[nodiscard]]``, the type | ||
| itself is now included in the diagnostic. | ||
|
|
||
| - Clang will now prefer the ``[[nodiscard]]`` declaration on function declarations over ``[[nodiscard]]`` | ||
| declaration on the return type of a function. Previously, when both have a ``[[nodiscard]]`` declaration attached, | ||
| the one on the return type would be preferred. This may affect the generated warning message: | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| struct [[nodiscard("Reason 1")]] S {}; | ||
| [[nodiscard("Reason 2")]] S getS(); | ||
| void use() | ||
| { | ||
| getS(); // Now diagnoses "Reason 2", previously diagnoses "Reason 1" | ||
| } | ||
|
|
||
| - Clang now diagnoses ``= delete("reason")`` extension warnings only in pedantic mode rather than on by default. (#GH109311). | ||
|
|
||
| - Clang now diagnoses missing return value in functions containing ``if consteval`` (#GH116485). | ||
|
|
||
| - Clang now correctly recognises code after a call to a ``[[noreturn]]`` constructor | ||
| as unreachable (#GH63009). | ||
|
|
||
| - Clang now omits shadowing warnings for parameter names in explicit object member functions (#GH95707). | ||
|
|
||
| - Improved error recovery for function call arguments with trailing commas (#GH100921). | ||
|
|
||
| - For an rvalue reference bound to a temporary struct with an integer member, Clang will detect constant integer overflow | ||
| in the initializer for the integer member (#GH46755). | ||
|
|
||
| - Fixed a false negative ``-Wunused-private-field`` diagnostic when a defaulted comparison operator is defined out of class (#GH116961). | ||
|
|
||
| - Clang now diagnoses dangling references for C++20's parenthesized aggregate initialization (#101957). | ||
|
|
||
| - Fixed a bug where Clang would not emit ``-Wunused-private-field`` warnings when an unrelated class | ||
| defined a defaulted comparison operator (#GH116270). | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| class A { | ||
| private: | ||
| int a; // warning: private field 'a' is not used, no diagnostic previously | ||
| }; | ||
|
|
||
| class C { | ||
| bool operator==(const C&) = default; | ||
| }; | ||
|
|
||
| - Clang now emits `-Wdangling-capture` diangostic when a STL container captures a dangling reference. | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| void test() { | ||
| std::vector<std::string_view> views; | ||
| views.push_back(std::string("123")); // warning | ||
| } | ||
|
|
||
| - Clang now emits a ``-Wtautological-compare`` diagnostic when a check for | ||
| pointer addition overflow is always true or false, because overflow would | ||
| be undefined behavior. | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| bool incorrect_overflow_check(const char *ptr, size_t index) { | ||
| return ptr + index < ptr; // warning | ||
| } | ||
|
|
||
| - Fix -Wdangling false positives on conditional operators (#120206). | ||
|
|
||
| - Fixed a bug where Clang hung on an unsupported optional scope specifier ``::`` when parsing | ||
| Objective-C. Clang now emits a diagnostic message instead of hanging. | ||
|
|
||
| - The :doc:`ThreadSafetyAnalysis` now supports passing scoped capabilities into functions: | ||
| an attribute on the scoped capability parameter indicates both the expected associated capabilities and, | ||
| like in the case of attributes on the function declaration itself, their state before and after the call. | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| #include "mutex.h" | ||
|
|
||
| Mutex mu1, mu2; | ||
| int a GUARDED_BY(mu1); | ||
|
|
||
| void require(MutexLocker& scope REQUIRES(mu1)) { | ||
| scope.Unlock(); | ||
| a = 0; // Warning! Requires mu1. | ||
| scope.Lock(); | ||
| } | ||
|
|
||
| void testParameter() { | ||
| MutexLocker scope(&mu1), scope2(&mu2); | ||
| require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead of 'mu1' | ||
| require(scope); // OK. | ||
| scope.Unlock(); | ||
| require(scope); // Warning! Requires mu1. | ||
| } | ||
|
|
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
| require(scope); // Warning! Requires mu1. | ||
| } | ||
|
|
||
| - Clang now forces attributes to not be scope parents (#84072). |
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 :) |
|
@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
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 :)