diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -657,6 +657,9 @@ (`#63169 _`) - Fix crash when casting an object to an array type. (`#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 --- a/clang/lib/Sema/JumpDiagnostics.cpp +++ b/clang/lib/Sema/JumpDiagnostics.cpp @@ -72,10 +72,9 @@ SmallVector Jumps; SmallVector IndirectJumps; - SmallVector AsmJumps; + SmallVector IndirectJumpTargets; SmallVector MustTailStmts; - SmallVector IndirectJumpTargets; - SmallVector 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 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(S)->getConstantTarget()) { - LabelAndGotoScopes[S] = ParentScope; - Jumps.push_back(S); - return; - } + if (cast(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(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(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(S); if (!(IS->isConstexpr() || IS->isConsteval() || @@ -666,6 +655,22 @@ continue; } + // If an asm goto jumps to a different 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(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(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 GotoJumps = IsAsmGoto ? AsmJumps : IndirectJumps; - if (GotoJumps.empty()) +void JumpScopeChecker::VerifyIndirectJumps() { + if (IndirectJumps.empty()) return; - SmallVector 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; SmallVector JumpScopes; { llvm::DenseMap 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 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 --- a/clang/test/Sema/asm-goto.cpp +++ b/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:; +} diff --git a/clang/test/SemaCXX/goto2.cpp b/clang/test/SemaCXX/goto2.cpp --- 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 @@ 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:; + } +}