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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D155342#4511879 <https://reviews.llvm.org/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<GCCAsmStmt>(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 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.
> but the second needs to branch to a call to test4cleanup().

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155342