[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<GCCAsmStmt>(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