efriedma added inline comments.
================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
+ if (auto *G = dyn_cast<GCCAsmStmt>(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 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155342/new/
https://reviews.llvm.org/D155342
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
