[PATCH] D69876: Allow output constraints on "asm goto"

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D69876#1791663 <https://reviews.llvm.org/D69876#1791663>, @void wrote:

> I'm not getting the `Undefined temporary symbol` in your example (even with optimizations):
>
>   [morbo at fawn:llvm-project] clang -o - -S ../bug.c
>


`-S` is going the AsmPrinter route. I observe the error with `-c`. The error goes away with `-c -no-integrated-as` pointing to an issue with LLVM.

In D69876#1791655 <https://reviews.llvm.org/D69876#1791655>, @void wrote:

> In D69876#1791475 <https://reviews.llvm.org/D69876#1791475>, @nickdesaulniers wrote:
>
> > 2. asm redirects control flow AFTER assigning to output. ie. ``` int foo(void) { int y; asm volatile goto ("mov 42, %0; ja %l1" : "=d"(y) ::: err); return y; err: return y; } ``` Why is this not valid in the case the indirect branch is taken?
>
>
> I'm not saying that it's *always* invalid, but we won't be able to guarantee correctness during code gen.


The language added in this patch states:

> It's important to note that outputs are valid only on the "fallthrough" branch.

So maybe `only` should be replaced?  Or maybe an additional statement informing the user that they must take care not to write inline assembly in such a way that transfers control flow to an indirect target from the inline asm, then expect to use the output?  Or states that it's explicitly undefined behavior whether the output is usable after the indirect branch is taken?

In D69876#1791475 <https://reviews.llvm.org/D69876#1791475>, @nickdesaulniers wrote:

> I need to think more about this, but I feel like people may see #2 as a reason to not use this feature and I have serious concerns about that.


I tried to dig up some prior art for this.  I found two cases that I think calm my concerns:

1. GCC bugs (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59615, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52381) particularly https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52381#c2, copied inline for emphasis:

>> Note that if output was only reliable on the fall-through path, it would already be useful.



2. LKML posts (https://lkml.org/lkml/2018/2/14/625, https://lkml.org/lkml/2018/2/14/656) particularly:

>> But extending on what gcc does, and allowing outputs (possibly valid
>>  in the fall-through case only, not in the cases where it jumps away to
>>  a label) would be a big improvement on what gcc does.

So I guess I should shelve my concerns for the minimum viable product.  It might be good to reach out to luto about what that `__get_user` implementation looks like, and how we might wire up support in the kernel for detecting compiler support for asm goto w/ output constraints, then being able to use it.  I'd be happy to help implement that.

One test I think it would be worthwhile to add here is to address the concern from the description of this bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59615); ie. that an output was properly clobbered.  None of the existing or added test cases seem to test this.  I'd expect such a test to read from a variable, use it as an output to an asm goto, then in the fallthrough use it, and we'd check that the updated value was used.  But I don't see any such tests in llvm/test/...

> Note that we're not going to be able to fix all of clang's inline assembly bugs with this feature. :-)

:P



================
Comment at: clang/test/Sema/asm-goto.cpp:35
+  // expected-error at +1 {{cannot jump from this asm goto statement to one of its possible targets}}
+  asm goto("jmp %l0;" ::::bar);
+    // expected-error at +1 {{cannot jump from this indirect goto statement to one of its possible targets}}
----------------
what's going on with the indentation?


================
Comment at: clang/test/Sema/asm-goto.cpp:44
-// expected-note at +1 {{possible target of indirect goto statement}}
-BAR:
   return;
----------------
Maybe the formatting changes to the tests (that don't add new tests or meaningfully change existing ones) above can just be pre-committed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69876/new/

https://reviews.llvm.org/D69876