[clang] f023f5c - [clang][JumpDiagnostics] ignore non-asm goto target scopes
Nick Desaulniers via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 20 20:05:01 PDT 2023
Author: Nick Desaulniers
Date: 2023-07-20T19:58:22-07:00
New Revision: f023f5cdb2e6c19026f04a15b5a935c041835d14
URL: https://github.com/llvm/llvm-project/commit/f023f5cdb2e6c19026f04a15b5a935c041835d14
DIFF: https://github.com/llvm/llvm-project/commit/f023f5cdb2e6c19026f04a15b5a935c041835d14.diff
LOG: [clang][JumpDiagnostics] ignore non-asm goto target scopes
The current behavior of JumpScopeChecker::VerifyIndirectOrAsmJumps was
to cross validate the scope of every jumping statement (goto, asm goto)
against the scope of every label (even if the label was not even a
possible target of the asm goto).
When we have multiple asm goto's with unique targets, we could trigger
false positive build errors complaining that labels that weren't even in
the asm goto's label list could not be jumped to. Example:
error: cannot jump from this asm goto statement to one of its possible targets
asm goto(""::::foo);
note: possible target of asm goto statement
bar:
^
Fixes: https://github.com/ClangBuiltLinux/linux/issues/1886
Reviewed By: void, jyu2, rjmccall
Differential Revision: https://reviews.llvm.org/D155342
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/JumpDiagnostics.cpp
clang/test/Sema/asm-goto.cpp
clang/test/SemaCXX/goto2.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bb9daddef8d198..05a7ec04d60c31 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -657,6 +657,9 @@ Bug Fixes in This Version
(`#63169 <https://github.com/llvm/llvm-project/issues/63169>_`)
- Fix crash when casting an object to an array type.
(`#63758 <https://github.com/llvm/llvm-project/issues/63758>_`)
+- Fixed false positive error diagnostic observed from mixing ``asm goto`` with
+ ``__attribute__((cleanup()))`` variables falsely warning that jumps to
+ non-targets would skip cleanup.
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/JumpDiagnostics.cpp b/clang/lib/Sema/JumpDiagnostics.cpp
index 4752f8af07d05c..5a6df56323a98b 100644
--- a/clang/lib/Sema/JumpDiagnostics.cpp
+++ b/clang/lib/Sema/JumpDiagnostics.cpp
@@ -72,10 +72,9 @@ class JumpScopeChecker {
SmallVector<Stmt*, 16> Jumps;
SmallVector<Stmt*, 4> IndirectJumps;
- SmallVector<Stmt*, 4> AsmJumps;
+ SmallVector<LabelDecl *, 4> IndirectJumpTargets;
SmallVector<AttributedStmt *, 4> MustTailStmts;
- SmallVector<LabelDecl*, 4> IndirectJumpTargets;
- SmallVector<LabelDecl*, 4> AsmJumpTargets;
+
public:
JumpScopeChecker(Stmt *Body, Sema &S);
private:
@@ -86,7 +85,7 @@ class JumpScopeChecker {
void BuildScopeInformation(Stmt *S, unsigned &origParentScope);
void VerifyJumps();
- void VerifyIndirectOrAsmJumps(bool IsAsmGoto);
+ void VerifyIndirectJumps();
void VerifyMustTailStmts();
void NoteJumpIntoScopes(ArrayRef<unsigned> ToScopes);
void DiagnoseIndirectOrAsmJump(Stmt *IG, unsigned IGScope, LabelDecl *Target,
@@ -115,8 +114,7 @@ JumpScopeChecker::JumpScopeChecker(Stmt *Body, Sema &s)
// Check that all jumps we saw are kosher.
VerifyJumps();
- VerifyIndirectOrAsmJumps(false);
- VerifyIndirectOrAsmJumps(true);
+ VerifyIndirectJumps();
VerifyMustTailStmts();
}
@@ -333,11 +331,8 @@ void JumpScopeChecker::BuildScopeInformation(Stmt *S,
// operand (to avoid recording the address-of-label use), which
// works only because of the restricted set of expressions which
// we detect as constant targets.
- if (cast<IndirectGotoStmt>(S)->getConstantTarget()) {
- LabelAndGotoScopes[S] = ParentScope;
- Jumps.push_back(S);
- return;
- }
+ if (cast<IndirectGotoStmt>(S)->getConstantTarget())
+ goto RecordJumpScope;
LabelAndGotoScopes[S] = ParentScope;
IndirectJumps.push_back(S);
@@ -354,27 +349,21 @@ void JumpScopeChecker::BuildScopeInformation(Stmt *S,
BuildScopeInformation(Var, ParentScope);
++StmtsToSkip;
}
+ goto RecordJumpScope;
+
+ case Stmt::GCCAsmStmtClass:
+ if (!cast<GCCAsmStmt>(S)->isAsmGoto())
+ break;
[[fallthrough]];
case Stmt::GotoStmtClass:
+ RecordJumpScope:
// Remember both what scope a goto is in as well as the fact that we have
// it. This makes the second scan not have to walk the AST again.
LabelAndGotoScopes[S] = ParentScope;
Jumps.push_back(S);
break;
- case Stmt::GCCAsmStmtClass:
- if (auto *GS = dyn_cast<GCCAsmStmt>(S))
- if (GS->isAsmGoto()) {
- // Remember both what scope a goto is in as well as the fact that we
- // have it. This makes the second scan not have to walk the AST again.
- LabelAndGotoScopes[S] = ParentScope;
- AsmJumps.push_back(GS);
- for (auto *E : GS->labels())
- AsmJumpTargets.push_back(E->getLabel());
- }
- break;
-
case Stmt::IfStmtClass: {
IfStmt *IS = cast<IfStmt>(S);
if (!(IS->isConstexpr() || IS->isConsteval() ||
@@ -666,6 +655,22 @@ void JumpScopeChecker::VerifyJumps() {
continue;
}
+ // If an asm goto jumps to a
diff erent scope, things like destructors or
+ // initializers might not be run which may be suprising to users. Perhaps
+ // this behavior can be changed in the future, but today Clang will not
+ // generate such code. Produce a diagnostic instead. See also the
+ // discussion here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110728.
+ if (auto *G = dyn_cast<GCCAsmStmt>(Jump)) {
+ for (AddrLabelExpr *L : G->labels()) {
+ LabelDecl *LD = L->getLabel();
+ unsigned JumpScope = LabelAndGotoScopes[G];
+ unsigned TargetScope = LabelAndGotoScopes[LD->getStmt()];
+ if (JumpScope != TargetScope)
+ DiagnoseIndirectOrAsmJump(G, JumpScope, LD, TargetScope);
+ }
+ continue;
+ }
+
// We only get indirect gotos here when they have a constant target.
if (IndirectGotoStmt *IGS = dyn_cast<IndirectGotoStmt>(Jump)) {
LabelDecl *Target = IGS->getConstantTarget();
@@ -694,17 +699,16 @@ void JumpScopeChecker::VerifyJumps() {
}
}
-/// VerifyIndirectOrAsmJumps - Verify whether any possible indirect goto or
-/// asm goto jump might cross a protection boundary. Unlike direct jumps,
-/// indirect or asm goto jumps count cleanups as protection boundaries:
-/// since there's no way to know where the jump is going, we can't implicitly
-/// run the right cleanups the way we can with direct jumps.
-/// Thus, an indirect/asm jump is "trivial" if it bypasses no
-/// initializations and no teardowns. More formally, an indirect/asm jump
-/// from A to B is trivial if the path out from A to DCA(A,B) is
-/// trivial and the path in from DCA(A,B) to B is trivial, where
-/// DCA(A,B) is the deepest common ancestor of A and B.
-/// Jump-triviality is transitive but asymmetric.
+/// VerifyIndirectJumps - Verify whether any possible indirect goto jump might
+/// cross a protection boundary. Unlike direct jumps, indirect goto jumps
+/// count cleanups as protection boundaries: since there's no way to know where
+/// the jump is going, we can't implicitly run the right cleanups the way we
+/// can with direct jumps. Thus, an indirect/asm jump is "trivial" if it
+/// bypasses no initializations and no teardowns. More formally, an
+/// indirect/asm jump from A to B is trivial if the path out from A to DCA(A,B)
+/// is trivial and the path in from DCA(A,B) to B is trivial, where DCA(A,B) is
+/// the deepest common ancestor of A and B. Jump-triviality is transitive but
+/// asymmetric.
///
/// A path in is trivial if none of the entered scopes have an InDiag.
/// A path out is trivial is none of the exited scopes have an OutDiag.
@@ -712,28 +716,24 @@ void JumpScopeChecker::VerifyJumps() {
/// Under these definitions, this function checks that the indirect
/// jump between A and B is trivial for every indirect goto statement A
/// and every label B whose address was taken in the function.
-void JumpScopeChecker::VerifyIndirectOrAsmJumps(bool IsAsmGoto) {
- SmallVector<Stmt*, 4> GotoJumps = IsAsmGoto ? AsmJumps : IndirectJumps;
- if (GotoJumps.empty())
+void JumpScopeChecker::VerifyIndirectJumps() {
+ if (IndirectJumps.empty())
return;
- SmallVector<LabelDecl *, 4> JumpTargets =
- IsAsmGoto ? AsmJumpTargets : IndirectJumpTargets;
// If there aren't any address-of-label expressions in this function,
// complain about the first indirect goto.
- if (JumpTargets.empty()) {
- assert(!IsAsmGoto && "only indirect goto can get here");
- S.Diag(GotoJumps[0]->getBeginLoc(),
+ if (IndirectJumpTargets.empty()) {
+ S.Diag(IndirectJumps[0]->getBeginLoc(),
diag::err_indirect_goto_without_addrlabel);
return;
}
- // Collect a single representative of every scope containing an
- // indirect or asm goto. For most code bases, this substantially cuts
- // down on the number of jump sites we'll have to consider later.
+ // Collect a single representative of every scope containing an indirect
+ // goto. For most code bases, this substantially cuts down on the number of
+ // jump sites we'll have to consider later.
using JumpScope = std::pair<unsigned, Stmt *>;
SmallVector<JumpScope, 32> JumpScopes;
{
llvm::DenseMap<unsigned, Stmt*> JumpScopesMap;
- for (Stmt *IG : GotoJumps) {
+ for (Stmt *IG : IndirectJumps) {
if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(IG)))
continue;
unsigned IGScope = LabelAndGotoScopes[IG];
@@ -749,7 +749,7 @@ void JumpScopeChecker::VerifyIndirectOrAsmJumps(bool IsAsmGoto) {
// label whose address was taken somewhere in the function.
// For most code bases, there will be only one such scope.
llvm::DenseMap<unsigned, LabelDecl*> TargetScopes;
- for (LabelDecl *TheLabel : JumpTargets) {
+ for (LabelDecl *TheLabel : IndirectJumpTargets) {
if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(TheLabel->getStmt())))
continue;
unsigned LabelScope = LabelAndGotoScopes[TheLabel->getStmt()];
diff --git a/clang/test/Sema/asm-goto.cpp b/clang/test/Sema/asm-goto.cpp
index 64addd9d75b6ef..52b9c3785fb5d7 100644
--- a/clang/test/Sema/asm-goto.cpp
+++ b/clang/test/Sema/asm-goto.cpp
@@ -59,3 +59,13 @@ int test3(int n)
loop:
return 0;
}
+
+void test4cleanup(int*);
+// No errors expected.
+void test4(void) {
+ asm goto(""::::l0);
+l0:;
+ int x __attribute__((cleanup(test4cleanup)));
+ asm goto(""::::l1);
+l1:;
+}
diff --git a/clang/test/SemaCXX/goto2.cpp b/clang/test/SemaCXX/goto2.cpp
index b42a6111824150..6fbe5b733f33c2 100644
--- a/clang/test/SemaCXX/goto2.cpp
+++ b/clang/test/SemaCXX/goto2.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fcxx-exceptions %s
// expected-no-diagnostics
//PR9463
@@ -46,3 +46,20 @@ int main() {
return 0;
}
+
+void asm_goto_try_catch() {
+ try {
+ __label__ label;
+ asm goto("" : : : : label);
+ label:;
+ } catch(...) {
+ __label__ label;
+ asm goto("" : : : : label);
+ label:;
+ };
+ if constexpr(false) {
+ __label__ label;
+ asm goto("" : : : : label);
+ label:;
+ }
+}
More information about the cfe-commits
mailing list