[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: ClangBuiltLinux/linux#1886

Reviewed By: void, jyu2, rjmccall

Differential Revision: https://reviews.llvm.org/D155342

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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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 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<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,46 +699,41 @@ 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.
 ///
 /// 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()];

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:;
+}

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:;
+  }
+}