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:
> 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.
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
