[clang][JumpDiagnostics] ignore non-asm goto target scopes

[nickdesaulniers]

Summary

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

[nickdesaulniers]

• add link to CBL

[nickdesaulniers]

• rebase

[nickdesaulniers]

• reword description

[nickdesaulniers]

• reword commit msg one more time

[jyu2]

LGTM.

[jyu2]

clang/lib/Sema/JumpDiagnostics.cpp

794

Just wonder why not just use TargetLabel instead?

[cor3ntin]

Thanks a lot for working on this.
This probably needs a release note entry

clang/lib/Sema/JumpDiagnostics.cpp

790–801

We don't need to make 3 copies!

791–800

I don't think the pointer adds that much, you can init capture by copy which simplifies a bit

clang/test/Sema/asm-goto.cpp

71

can you add more tests? Maybe something like that

cpp
void f() {
  try{
    __label__ label;
    asm goto("" : : : : label);
    label:;
  }
  catch(...){
    __label__ label;
    asm goto("" : : : : label);
    label:;
  };
  if constexpr(false) {
    __label__ label;
    asm goto("" : : : : label);
    label:;
  }
}

[cor3ntin]

clang/lib/Sema/JumpDiagnostics.cpp

791–800

I don't think the pointer adds that much, you can init capture by copy which simplifies a bit

s/pointer/comment

[rjmccall]

Wait, the whole point of this algorithm is that we have to do this whole complicated linear check against every label whose address is taken because we don't know where it's going. If we have a list of all the possible labels that the asm goto might jump to, why are we building a map of all the labels and then filtering out the ones that aren't listed? We should just be checking the listed destinations with the stricter triviality rule that indirect-goto uses.

[nickdesaulniers]

In D155342#4504812, @rjmccall wrote:

Wait, the whole point of this algorithm is that we have to do this whole complicated linear check against every label whose address is taken because we don't know where it's going. If we have a list of all the possible labels that the asm goto might jump to, why are we building a map of all the labels and then filtering out the ones that aren't listed? We should just be checking the listed destinations with the stricter triviality rule that indirect-goto uses.

Great point. Looks like I should probably be calling CheckJump from JumpScopeChecker::VerifyJumps instead. Let me work on that refactoring instead.

[nickdesaulniers]

• test just asm goto targets, as per @rjmccall (this lead to many cleanups)
• add release note and test case as per @cor3ntin
• remove changes to JumpScopeChecker::BuildScopeInformation

[nickdesaulniers]

clang/test/Sema/asm-goto.cpp

71

Done, but I have to put it somewhere else because:

1. exceptions are disabled in clang/test/Sema/*. This needs to go in clang/test/SemaCXX/
2. if constexpr is a C++17 addition.

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

75

AsmJumps can now be removed, too.

[nickdesaulniers]

• remove AsmJumps

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

730

should remove this reference to asm goto as well.

[nickdesaulniers]

• remove stale asm goto comment

[rjmccall]

Thanks, that's way better. Just a couple minor requests.

clang/docs/ReleaseNotes.rst

629 ↗

(On Diff #541169)

clang/lib/Sema/JumpDiagnostics.cpp

363

You can pull the GCCAsmStmtClass case right above this, make the cast unconditional (if (!cast<GCCAsmStmt>(S)->isAsmGoto()) break;), and then fall through into the GotoStmt case.

[nickdesaulniers]

• fix typo in release doc

clang/lib/Sema/JumpDiagnostics.cpp

363

I could hoist + use [[fallthrough]] but note that the case above Stmt::SwitchStmtClass also currently uses [[fallthrough]] here as well; so I would still need the dyn_cast.

With that in mind, do you still prefer the hoisting? I don't have a preference, but wanted to triple check that with you.

[rjmccall]

clang/lib/Sema/JumpDiagnostics.cpp

363

Ah, right. Personally, I would probably put this common path in a helper function, but you could also just duplicate it since it's so small. This also seems like an acceptable use-case for goto if you can stomach that — with a good label, it should still be very readable.

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

363

Is https://reviews.llvm.org/D155522 what you had in mind? If so, then I'll squash it into this, otherwise can you state differently what you had in mind?

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

363

oh, rereading what you wrote, I think I understand what you mean with gotos. Let me post a diff of that, for clarification.

[rjmccall]

I do like the look of https://reviews.llvm.org/D155522, yeah, since we have so many of these that aren't top-level in a case anyway.

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

363

https://reviews.llvm.org/D155525 perhaps?

[nickdesaulniers]

• squash D155525 as per @rjmccall

[nickdesaulniers]

Thanks for the reviews and advice. Any parting thoughts @rjmccall ?

[rjmccall]

clang/lib/Sema/JumpDiagnostics.cpp

669

I think it would be good to leave a comment here like this:

We know the possible destinations of an asm goto, but we don'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.

[nickdesaulniers]

• add comment as per @rjmccall

clang/lib/Sema/JumpDiagnostics.cpp

669

Depending on your definition of "we" (clang vs. llvm), llvm does have the ability to add code along those edges.

See llvm/lib/CodeGen/CallBrPrepare.cpp. I'll clarify in your comment that "clang does not split critical edges during code generation of llvm ... "

[rjmccall]

clang/lib/Sema/JumpDiagnostics.cpp

669

Okay, so, I don't really know much about this feature. I was thinking that the branch might go directly into some other assembly block, which would not be splittable. If the branch just goes to an arbitrary basic block in IR, then it would be straightforward for IRGen to just resolve the destination blocks for asm goto labels to some new block that does a normal goto to that label. If we did that, we wouldn't need extra restrictions here at all and could just check this like any other direct branch.

We don't need to do that work right away, but the comment should probably reflect the full state of affairs — "but clang's IR generation does not currently know how to add code along these control flow edges, so we have to diagnose jumps both in and out of scopes, like we do with indirect goto. If we ever add that ability to IRGen, this code could check these jumps just like ordinary gotos."

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

669

Okay, so, I don't really know much about this feature.

"Run this block of asm, then continue to either the next statement or one of the explicit labels in the label list."

---

That comment still doesn't seem quite right to me.

asm goto is more like a direct goto or a switch in the sense that the cases or possible destination are known at compile time; that's not like indirect goto where you're jumping to literally anywhere.

We need to check the scopes like we would for direct goto, because we don't want to bypass non-trivial destructors.

---

Interestingly, it looks like some of the cases inclang/test/Sema/asm-goto.cpp, g++ permits, if you use the -fpermissive flag. Clang doesn't error that it doesn't recognize that flag, but it doesn't seem to do anything in clang, FWICT playing with it in godbolt.

---

That said, I would have thought

void test4cleanup(int*);
// No errors expected.
void test4(void) {
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    asm goto("# %l0"::::l0);
}

To work with this change, but we still produce:

x.c:6:5: error: cannot jump from this asm goto statement to one of its possible targets
    6 |     asm goto("# %l0"::::l0);
      |     ^
x.c:4:1: note: possible target of asm goto statement
    4 | l0:;
      | ^
x.c:5:9: note: jump exits scope of variable with __attribute__((cleanup))
    5 |     int x __attribute__((cleanup(test4cleanup)));
      |         ^

Aren't those in the same scope though? I would have expected that to work just as if we had a direct goto l0 rather than the asm goto.

[rjmccall]

We need to check the scopes like we would for direct goto, because we don't want to bypass non-trivial destructors.

I think this is the basic misunderstanding here. Direct goto is allowed to jump out of scopes; it just runs destructors on the way. It is only a direct branch to the target block if there are no destructors along the path.

Indirect goto is not allowed to jump out of scopes because, in general, the labels could be in different scopes with different sets of destructors required, and so the only way we could run the right set of destructors along the way would be to dynamically compare the label value with specific labels, which would defeat the point of the feature. (This is unnecessary if all the labels are in the same scope, which is typical, but nobody has felt like extending the feature to recognize that pattern and be more general.)

[efriedma]

clang/lib/Sema/JumpDiagnostics.cpp

669

(There's some history here that the original implementation of asm goto treated it semantically more like an indirect goto, including the use of address-of-labels; for a variety of reasons, we changed it so it's more like a switch statement.)

Suppose we have:

void test4cleanup(int*);
void test4(void) {
    asm goto("# %l0"::::l0);
l0:;
    int x __attribute__((cleanup(test4cleanup)));
    asm goto("# %l0"::::l0);
}

To make this work correctly, the first goto needs to branch directly to the destination, but the second needs to branch to a call to test4cleanup(). It's probably not that hard to implement: instead of branching directly to the destination bb, each edge out of the callbr needs to branch to a newly created block, and that block needs to EmitBranchThroughCleanup() to the final destination. (We create such blocks anyway to handle output values, but the newly created blocks branch directly to the destination BasicBlock instead of using EmitBranchThroughCleanup().)

But until we implement that, we need the error message so we don't miscompile.

[nickdesaulniers]

• rewrite comment based on feedback from @rjmccall and @efriedma

[nickdesaulniers]

In D155342#4511879, @rjmccall wrote:

We need to check the scopes like we would for direct goto, because we don't want to bypass non-trivial destructors.

I think this is the basic misunderstanding here. Direct goto is allowed to jump out of scopes; it just runs destructors on the way. It is only a direct branch to the target block if there are no destructors along the path.

Indirect goto is not allowed to jump out of scopes because, in general, the labels could be in different scopes with different sets of destructors required

Ah, got it.

clang/lib/Sema/JumpDiagnostics.cpp

669

but the second needs to branch to a call to test4cleanup().

GCC does not behave that way (i.e. if the branch is taken from the asm goto to l0, test4cleanup is not run). In fact, if I remove the call to DiagnoseIndirectOrAsmJump below, we generate the same control flow that GCC 12 does. https://godbolt.org/z/Y6en3YsY1

Perhaps one could argue "that's surprising" or "not correct" but if we were to have such a difference then that would probably preclude the use of the unholy combination of asm goto and __attribute__((cleanup())) (famous last words).

Let me try again with the comment based on feedback thus far.

[efriedma]

clang/lib/Sema/JumpDiagnostics.cpp

669

I guess that's an argument for keeping around this diagnostic, at least for now.

Can you file a bug against gcc?

[jyknight]

clang/lib/Sema/JumpDiagnostics.cpp

669

That looks like a bug in GCC to me. Can you open a bug report against GCC with your test-case, and refer to the bug here?

I think we should not modify Clang to implement any non-error behavior until the GCC bug is addressed.

[rjmccall]

Yes, this diagnostic implements a core semantic rule and must be emitted. There are some situations where Clang will actually generate malformed IR (violating dominance rules) if you fail to diagnose jump violations correctly. The IR you get when there's a jump over a destructor is well-formed, but it's still arguably* a miscompile and must be diagnosed.

(*) It's a miscompile unless there's a rule saying it's U.B. to jump out of a scope. I don't see such a rule in any of the docs about this feature.

[nickdesaulniers]

• add link to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110728

[rjmccall]

LGTM

[nathanchance]

Thanks, I can confirm that the __attribute__((__cleanup__(...))) errors that we observed with Peter's work-in-progress guards series are resolved and that all the errors I observed when building the kernel after https://reviews.llvm.org/D154696 was merged are no longer present when that change is re-applied on top of this one.

[shafik]

clang/lib/Sema/JumpDiagnostics.cpp

680

Do we have a test that checks this diagnostic?

[nickdesaulniers]

clang/lib/Sema/JumpDiagnostics.cpp

680

Yes, all of the existing tests in clang/test/Sema/asm-goto.cpp are a result of this statement.

clang/lib/Sema/JumpDiagnostics.cpp

GotoScope(unsigned parentScope, unsigned InDiag, unsigned OutDiag,
       : ParentScope(parentScope), InDiag(InDiag), OutDiag(OutDiag), Loc(L) {}
   };
 
   SmallVector<GotoScope, 48> Scopes;
   llvm::DenseMap<Stmt*, unsigned> LabelAndGotoScopes;
   SmallVector<Stmt*, 16> Jumps;
 
   SmallVector<Stmt*, 4> IndirectJumps;
   SmallVector<Stmt*, 4> AsmJumps;
   SmallVector<AttributedStmt *, 4> MustTailStmts;
   SmallVector<LabelDecl*, 4> IndirectJumpTargets;
   SmallVector<LabelDecl*, 4> AsmJumpTargets;
 public:
   JumpScopeChecker(Stmt *Body, Sema &S);
 private:
   void BuildScopeInformation(Decl *D, unsigned &ParentScope);
   void BuildScopeInformation(VarDecl *D, const BlockDecl *BDecl,
if (VarDecl *Var = cast<SwitchStmt>(S)->getConditionVariable()) {
       ++StmtsToSkip;
     }
     [[fallthrough]];
 
   case Stmt::GotoStmtClass:
     // 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())
+        for (AddrLabelExpr *E : GS->labels())
           AsmJumpTargets.push_back(E->getLabel());
       }
     break;
 
   case Stmt::IfStmtClass: {
     IfStmt *IS = cast<IfStmt>(S);
     if (!(IS->isConstexpr() || IS->isConsteval() ||
           IS->isObjCAvailabilityCheck()))
if (GotoStmt *GS = dyn_cast<GotoStmt>(Jump)) {
                   diag::err_goto_into_protected_scope,
                   diag::ext_goto_into_protected_scope,
                   diag::warn_cxx98_compat_goto_into_protected_scope);
       }
       CheckGotoStmt(GS);
       continue;
     }
 
     // We only get indirect gotos here when they have a constant target.
     if (IndirectGotoStmt *IGS = dyn_cast<IndirectGotoStmt>(Jump)) {
       LabelDecl *Target = IGS->getConstantTarget();
       CheckJump(IGS, Target->getStmt(), IGS->getGotoLoc(),
                 diag::err_goto_into_protected_scope,
                 diag::ext_goto_into_protected_scope,
                 diag::warn_cxx98_compat_goto_into_protected_scope);
       continue;
     }
 
     SwitchStmt *SS = cast<SwitchStmt>(Jump);
     for (SwitchCase *SC = SS->getSwitchCaseList(); SC;
          SC = SC->getNextSwitchCase()) {
       if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(SC)))
         continue;
       SourceLocation Loc;
       if (CaseStmt *CS = dyn_cast<CaseStmt>(SC))
         Loc = CS->getBeginLoc();
       else if (DefaultStmt *DS = dyn_cast<DefaultStmt>(SC))
         Loc = DS->getBeginLoc();
void JumpScopeChecker::VerifyIndirectOrAsmJumps(bool IsAsmGoto) {
   // complain about the first indirect goto.
   if (JumpTargets.empty()) {
     assert(!IsAsmGoto && "only indirect goto can get here");
     S.Diag(GotoJumps[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.
   using JumpScope = std::pair<unsigned, Stmt *>;
   SmallVector<JumpScope, 32> JumpScopes;
   {
     llvm::DenseMap<unsigned, Stmt*> JumpScopesMap;
     for (Stmt *IG : GotoJumps) {
       if (CHECK_PERMISSIVE(!LabelAndGotoScopes.count(IG)))
         continue;
while (true) {
       // Stop if we can't trivially enter the current scope.
       if (Scopes[Min].InDiag) break;
 
       Min = Scopes[Min].ParentScope;
     }
 
     // Walk through all the jump sites, checking that they can trivially
     // reach this label scope.
     for (auto [JumpScope, JumpStmt] : JumpScopes) {
+      // This unnecessary copy is because:
+      // warning: captured structured bindings are a C++20 extension
+      // [-Wc++20-extensions]
+      LabelDecl *TL = TargetLabel;
+      // Is TargetLabel one of the targets of the JumpStmt? If not, then skip
+      // it.
+      if (IsAsmGoto &&
+          llvm::none_of(cast<GCCAsmStmt>(JumpStmt)->labels(),
+                        [=](AddrLabelExpr *E) { return E->getLabel() == TL; }))
+        continue;
+
       unsigned Scope = JumpScope;
       // Walk out the "scope chain" for this scope, looking for a scope
       // we've marked reachable.  For well-formed code this amortizes
       // to O(JumpScopes.size() / Scopes.size()):  we only iterate
       // when we see something unmarked, and in well-formed code we
       // mark everything we iterate past.
       bool IsReachable = false;
       while (true) {

clang/test/Sema/asm-goto.cpp

int test3(int n)
   // expected-note@+1 {{possible target of asm goto statement}}
   return ({int a[n];label_true: 2;});
   // expected-note@+1 {{jump bypasses initialization of variable length array}}
   int b[n];
 // expected-note@+1 {{possible target of asm goto statement}}
 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:;
+}