[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf023f5cdb2e6: [clang][JumpDiagnostics] ignore non-asm goto 
target scopes (authored by nickdesaulniers).

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

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:669
+if (JumpScope != TargetScope)
+  DiagnoseIndirectOrAsmJump(G, JumpScope, LD, TargetScope);
+  }

shafik wrote:
> Do we have a test that checks this diagnostic?
Yes, //all// of the existing tests in clang/test/Sema/asm-goto.cpp are a result 
of this statement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:669
+if (JumpScope != TargetScope)
+  DiagnoseIndirectOrAsmJump(G, JumpScope, LD, TargetScope);
+  }

Do we have a test that checks this diagnostic?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 542081.
nickdesaulniers marked 4 inline comments as done.
nickdesaulniers added a comment.

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


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

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

efriedma wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > rjmccall wrote:
> > > > > nickdesaulniers wrote:
> > > > > > rjmccall wrote:
> > > > > > > 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.
> > > > > > 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 ... "
> > > > > 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 
> > > > > `goto`s."
> > > > > 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`.
> > > (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 

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> efriedma wrote:
> > nickdesaulniers wrote:
> > > rjmccall wrote:
> > > > nickdesaulniers wrote:
> > > > > rjmccall wrote:
> > > > > > 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.
> > > > > 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 ... "
> > > > 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 `goto`s."
> > > > 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`.
> > (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

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.




Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

efriedma wrote:
> nickdesaulniers wrote:
> > rjmccall wrote:
> > > nickdesaulniers wrote:
> > > > rjmccall wrote:
> > > > > 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.
> > > > 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 ... "
> > > 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 `goto`s."
> > > 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`.
> (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 
> destinat

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541705.
nickdesaulniers added a comment.

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


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 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,21 @@
   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.
+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 +

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> rjmccall wrote:
> > nickdesaulniers wrote:
> > > rjmccall wrote:
> > > > 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.
> > > 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 
> > > ... "
> > 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 `goto`s."
> > 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`.
(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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mail

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> 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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

rjmccall wrote:
> nickdesaulniers wrote:
> > rjmccall wrote:
> > > 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.
> > 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 ... "
> 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 `goto`s."
> 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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

nickdesaulniers wrote:
> rjmccall wrote:
> > 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.
> 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 ... "
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 `goto`s."


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

rjmccall wrote:
> 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.
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 ... "


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

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 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;
 }
 
+// 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(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)) {
   L

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
 
+if (auto *G = dyn_cast(Jump)) {
+  for (AddrLabelExpr *L : G->labels()) {

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers marked 2 inline comments as done.
nickdesaulniers added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541619.
nickdesaulniers added a comment.

- squash D155525  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 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,17 @@
   continue;
 }
 
+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 +694,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 th

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > rjmccall wrote:
> > > nickdesaulniers wrote:
> > > > rjmccall wrote:
> > > > > You can pull the `GCCAsmStmtClass` case right above this, make the 
> > > > > cast unconditional (`if (!cast(S)->isAsmGoto()) break;`), 
> > > > > and then fall through into the GotoStmt case.
> > > > 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.
> > > 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.
> > 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?
> oh, rereading what you wrote, I think I understand what you mean with 
> `goto`s. Let me post a diff of that, for clarification.
https://reviews.llvm.org/D155525 perhaps?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

nickdesaulniers wrote:
> rjmccall wrote:
> > nickdesaulniers wrote:
> > > rjmccall wrote:
> > > > You can pull the `GCCAsmStmtClass` case right above this, make the cast 
> > > > unconditional (`if (!cast(S)->isAsmGoto()) break;`), and 
> > > > then fall through into the GotoStmt case.
> > > 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.
> > 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.
> 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?
oh, rereading what you wrote, I think I understand what you mean with `goto`s. 
Let me post a diff of that, for clarification.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

rjmccall wrote:
> nickdesaulniers wrote:
> > rjmccall wrote:
> > > You can pull the `GCCAsmStmtClass` case right above this, make the cast 
> > > unconditional (`if (!cast(S)->isAsmGoto()) break;`), and then 
> > > fall through into the GotoStmt case.
> > 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.
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

nickdesaulniers wrote:
> rjmccall wrote:
> > You can pull the `GCCAsmStmtClass` case right above this, make the cast 
> > unconditional (`if (!cast(S)->isAsmGoto()) break;`), and then 
> > fall through into the GotoStmt case.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

rjmccall wrote:
> You can pull the `GCCAsmStmtClass` case right above this, make the cast 
> unconditional (`if (!cast(S)->isAsmGoto()) break;`), and then 
> fall through into the GotoStmt case.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541178.
nickdesaulniers added a comment.

- fix typo in release doc


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 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();
 }
 
@@ -357,24 +355,16 @@
 [[fallthrough]];
 
   case Stmt::GotoStmtClass:
+  case Stmt::GCCAsmStmtClass:
+if (auto *GS = dyn_cast(S))
+  if (!GS->isAsmGoto())
+break;
 // 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 +656,17 @@
   continue;
 }
 
+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 +695,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, i

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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




Comment at: clang/docs/ReleaseNotes.rst:629
+- Fixed false positive error diagnostic observed from mixing ``asm goto`` with
+  ``__attribute__((cleanup()))`` variables falsly warning that jumps to
+  non-targets would skip cleanup.





Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361
+  if (!GS->isAsmGoto())
+break;
 // Remember both what scope a goto is in as well as the fact that we have

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541173.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- remove stale asm goto comment


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 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();
 }
 
@@ -357,24 +355,16 @@
 [[fallthrough]];
 
   case Stmt::GotoStmtClass:
+  case Stmt::GCCAsmStmtClass:
+if (auto *GS = dyn_cast(S))
+  if (!GS->isAsmGoto())
+break;
 // 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 +656,17 @@
   continue;
 }
 
+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 +695,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
+/

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:726
   // 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.

should remove this reference to `asm goto` as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541169.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- remove AsmJumps


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 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();
 }
 
@@ -357,24 +355,16 @@
 [[fallthrough]];
 
   case Stmt::GotoStmtClass:
+  case Stmt::GCCAsmStmtClass:
+if (auto *GS = dyn_cast(S))
+  if (!GS->isAsmGoto())
+break;
 // 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 +656,17 @@
   continue;
 }
 
+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 +695,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 pro

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:75
   SmallVector IndirectJumps;
   SmallVector AsmJumps;
   SmallVector MustTailStmts;

`AsmJumps` can now be removed, too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/test/Sema/asm-goto.cpp:71
+l1:;
+}

cor3ntin wrote:
> 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:;
>   }
> }
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 541139.
nickdesaulniers marked 5 inline comments as done.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

- 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


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
@@ -74,8 +74,8 @@
   SmallVector IndirectJumps;
   SmallVector AsmJumps;
   SmallVector MustTailStmts;
-  SmallVector IndirectJumpTargets;
-  SmallVector AsmJumpTargets;
+  SmallVector IndirectJumpTargets;
+
 public:
   JumpScopeChecker(Stmt *Body, Sema &S);
 private:
@@ -86,7 +86,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 +115,7 @@
 
   // Check that all jumps we saw are kosher.
   VerifyJumps();
-  VerifyIndirectOrAsmJumps(false);
-  VerifyIndirectOrAsmJumps(true);
+  VerifyIndirectJumps();
   VerifyMustTailStmts();
 }
 
@@ -357,24 +356,16 @@
 [[fallthrough]];
 
   case Stmt::GotoStmtClass:
+  case Stmt::GCCAsmStmtClass:
+if (auto *GS = dyn_cast(S))
+  if (!GS->isAsmGoto())
+break;
 // 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 +657,17 @@
   continue;
 }
 
+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 +696,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,

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:791-800
+  // 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 &&

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

s/pointer/comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

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




Comment at: clang/lib/Sema/JumpDiagnostics.cpp:790
 // reach this label scope.
 for (auto [JumpScope, JumpStmt] : JumpScopes) {
+  // This unnecessary copy is because:

We don't need to make 3 copies!



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:791-800
+  // 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 &&

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



Comment at: clang/test/Sema/asm-goto.cpp:71
+l1:;
+}

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added inline comments.



Comment at: clang/lib/Sema/JumpDiagnostics.cpp:794
+  // [-Wc++20-extensions]
+  LabelDecl *TL = TargetLabel;
+  // Is TargetLabel one of the targets of the JumpStmt? If not, then skip

Just wonder why not just use TargetLabel instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 accepted this revision.
jyu2 added a comment.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 540586.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- reword commit msg one more time


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

Files:
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/test/Sema/asm-goto.cpp


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 540585.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- reword description


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

Files:
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/test/Sema/asm-goto.cpp


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 540584.
nickdesaulniers added a comment.

- rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

Files:
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/test/Sema/asm-goto.cpp


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers updated this revision to Diff 540574.
nickdesaulniers edited the summary of this revision.
nickdesaulniers added a comment.

- add link to CBL


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155342/new/

https://reviews.llvm.org/D155342

Files:
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/test/Sema/asm-goto.cpp


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
Herald added a project: All.
nickdesaulniers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Otherwise we observe false positive warnings claiming that an asm goto
cannot jump to another label in a different scope when that label isn't
even a target of the asm goto.

This was a bug introduced back in initial support for asm goto,
commit b8fee677bf8e ("Re-check in clang support gun asm goto after fixing 
tests.")


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155342

Files:
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/test/Sema/asm-goto.cpp


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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


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
@@ -370,7 +370,7 @@
 // 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;
@@ -788,6 +788,17 @@
 // 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(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits