jyknight added inline comments.
================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
+ if (auto *G = dyn_cast<GCCAsmStmt>(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 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.
> I guess that's an argument for keeping around this diagnostic, at least for
> now.
>
> Can you file a bug against gcc?
That looks like a bug in GCC to me. Can you open a bug report against GCC with
your test-case, and refer to the bug here?
I think we should not modify Clang to implement any non-error behavior until
the GCC bug is addressed.
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
