nickdesaulniers added inline comments.
================
Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658
+ if (auto *G = dyn_cast<GCCAsmStmt>(Jump)) {
+ for (AddrLabelExpr *L : G->labels()) {
----------------
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`.
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
