nickdesaulniers updated this revision to Diff 541639.
nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.
- add comment as per @rjmccall
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155342/new/
https://reviews.llvm.org/D155342
Files:
clang/docs/ReleaseNotes.rst
clang/lib/Sema/JumpDiagnostics.cpp
clang/test/Sema/asm-goto.cpp
clang/test/SemaCXX/goto2.cpp
Index: clang/test/SemaCXX/goto2.cpp
===================================================================
--- clang/test/SemaCXX/goto2.cpp
+++ 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 @@
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:;
+ }
+}
Index: clang/test/Sema/asm-goto.cpp
===================================================================
--- clang/test/Sema/asm-goto.cpp
+++ clang/test/Sema/asm-goto.cpp
@@ -59,3 +59,13 @@
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:;
+}
Index: clang/lib/Sema/JumpDiagnostics.cpp
===================================================================
--- clang/lib/Sema/JumpDiagnostics.cpp
+++ clang/lib/Sema/JumpDiagnostics.cpp
@@ -72,10 +72,9 @@
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 @@
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 @@
// Check that all jumps we saw are kosher.
VerifyJumps();
- VerifyIndirectOrAsmJumps(false);
- VerifyIndirectOrAsmJumps(true);
+ VerifyIndirectJumps();
VerifyMustTailStmts();
}
@@ -333,11 +331,8 @@
// 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 @@
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 @@
continue;
}
+ // We know the possible destinations of an asm goto, but clang does not
+ // split critical edges during codegen of LLVM IR. Thus, clang doesn't have
+ // the ability to add code along those control flow edges, so we have to
+ // diagnose jumps both in and out of scopes, just like we do with an
+ // indirect goto.
+ 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 @@
}
}
-/// 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 @@
/// 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 @@
// 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()];
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -634,6 +634,9 @@
that construct (`#62133 <https://github.com/llvm/llvm-project/issues/38717>_`).
- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
(`#63169 <https://github.com/llvm/llvm-project/issues/63169>_`)
+- 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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
