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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
