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

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

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

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


Okay. I'll check into it.

> 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?

I'm afraid that if we open the door for people to use outputs on indirect branches when it's not fully supported then we'll have to support that "feature" forever, when in reality it's an accident of implementation. It might just be best to say that it's undefined behavior.

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

I think clang normally uses something like `has_attribute` for things like this. Could you contact luto and ask for an example?

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

I'll add some tests for these.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69876