Support output constraints on "asm goto"

[void]

Summary

Clang's "asm goto" feature didn't initially support outputs constraints. That
was the same behavior as gcc's implementation. The decision by gcc not to
support outputs was based on a restriction in their IR regarding terminators.
LLVM doesn't restrict terminators from returning values (e.g. 'invoke'), so
it made sense to support this feature.

Output values are valid only on the 'fallthrough' path. If an output value's used
on an indirect branch, then it's 'poisoned'.

In theory, outputs *could* be valid on the 'indirect' paths, but it's very
difficult to guarantee that the original semantics would be retained. E.g.
because indirect labels could be used as data, we wouldn't be able to split
critical edges in situations where two 'callbr' instructions have the same
indirect label, because the indirect branch's destination would no longer be
the same.

[void]

Friendly ping. :-)

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

769

GCCAsmStmt inherits from AsmStmt which has an outputs() method for returning an iterator. Please use that an a range based for loop.

814

Don't declare as as const, then you won't need this const_cast.

[void]

Remove a "const_cast" and use iterators for output constraints.

[void]

Changes made. PTAL

[nickdesaulniers]

clang/lib/Analysis/UninitializedValues.cpp

770

this is still not a range based for loop.

Does:

for (const auto &O : as->outputs())
  if (const VarDecl *VD = findVar(O).getDecl())
    vals[VD] = Initialized;

not work?

[void]

Adjust loop.

[void]

clang/lib/Analysis/UninitializedValues.cpp

770

Done.

[void]

Any further comments?

[rnk]

Changes seem fine to me, FWIW.
+@rsmith

clang/test/Analysis/uninit-asm-goto.cpp

11

This could use a test for the case where an input is uninitialized, and where an uninitialized value is used along the error path.

[void]

clang/test/Analysis/uninit-asm-goto.cpp

11

I have a follow-up patch that I'll upload soon that warns about uninitialized variable use on the indirect paths. I wanted to separate it to keep this patch size down. If you'd prefer I can just add it.

[void]

Note to reviewers: I would *really* like to get this feature into the 10.0.0 release. That's coming up in January. Do you think you'll have your reviews finished by then?

[nickdesaulniers]

Thinking hard about this, I think there's four cases we really need to think hard about:

1. asm redirects control flow BEFORE assigning to output. ie.

int foo(void) {
  int y;
  asm volatile goto ("ja %l1" : "=r"(y) ::: err);
  return y;
err:
  return y;
}

I think we can chalk this up to user error; stupid asm in, stupid asm out. If the inline asm never assigns to the output, regardless of how the assembly block is exited, there's nothing we can do. Your child patch (https://reviews.llvm.org/D71314) warns in the second return, though the first return is equally invalid.

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?

The two other cases I can think of are in regards to conflicting constraints. Consider for example the x86 machine specific output constraints (https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints):

3. shared indirect destinations with differing/conflicting constraints

void foo(void) {
    int y;
    asm volatile ("mov 42, %0" : "=d"(y));
}
void bar(void) {
    int y;
    asm volatile ("mov 42, %0" : "=c"(y));
}
void baz(void) {
    int y;
    asm volatile goto ("mov 42, %0; ja %1": "=d"(y) ::: quux);
    asm volatile goto ("mov 42, %0; ja %1": "=c"(y) ::: quux);
    return;
quux:
    return;
}

foo puts 42 in %edx. bar puts 42 in %ecx. Where should baz put 42? Your current patch set produces: error: Undefined temporary symbol.

4. conflicts between register variables and output constraints.

void foo(void) {
    register int y asm("edx") = 0;
    asm volatile goto ("mov 42, %0; ja %l1" : "=c"(y) ::: bar);
bar:
    return;
}
void baz(void) {
    register int y asm("edx") = 0;
    asm volatile ("mov 42, %0" : "=c"(y));
}

The output constraint for asm goto says put the output in %ecx, yet the variable was declared as having register storage in %edx.

Looks like Clang already has bugs or at least disagrees with GCC (for the simpler case of baz, but the problem still exists for foo). Filed https://bugs.llvm.org/show_bug.cgi?id=44328.

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.

clang/docs/LanguageExtensions.rst

1280

Would you mind removing the above in this sentence. I initially parsed this as the block above err, which is not correct.

[void]

In D69876#1791475, @nickdesaulniers wrote:

Thinking hard about this, I think there's four cases we really need to think hard about:

1. asm redirects control flow BEFORE assigning to output. ie.
   
   ` int foo(void) { int y; asm volatile goto ("ja %l1" : "=r"(y) ::: err); return y; err: return y; } `
   
   I think we can chalk this up to user error; stupid asm in, stupid asm out. If the inline asm never assigns to the output, regardless of how the assembly block is exited, there's nothing we can do. Your child patch (https://reviews.llvm.org/D71314) warns in the second return, though the first return is equally invalid.

Right. This is human error. Unless we can parse the inline assembly (not something I'm suggesting) we won't be able to help them here.

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. For example, if you have something like:

int foo(int x) {
  int y;
  if (x < 42)
    asm volatile goto ("mov 42, %0; ja %l1" : "=D"(y) ::: err);
  else
    asm volatile goto ("mov 0, %0; ja %l1" : "=S"(y) ::: err);
  return y;
err:
  return y;
}

we can't necessarily code gen this correctly for the err block. There was a long thread on ways we could try to do this, but they were all very sub-standard.

The two other cases I can think of are in regards to conflicting constraints. Consider for example the x86 machine specific output constraints (https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html#Machine-Constraints):

3. shared indirect destinations with differing/conflicting constraints ` void foo(void) { int y; asm volatile ("mov 42, %0" : "=d"(y)); } void bar(void) { int y; asm volatile ("mov 42, %0" : "=c"(y)); } void baz(void) { int y; asm volatile goto ("mov 42, %0; ja %1": "=d"(y) ::: quux); asm volatile goto ("mov 42, %0; ja %1": "=c"(y) ::: quux); return; quux: return; } ` foo puts 42 in %edx. bar puts 42 in %ecx. Where should baz put 42? Your current patch set produces: error: Undefined temporary symbol.

Whatever is done for inline assembly that's not "goto" is done here. Or at least should be. I'll fix the error message. But in essence it will move the correct value into the correct register. In this case, it should move 42 into both %edx and %ecx. The temporary symbol issue is most likely unrelated.

4. conflicts between register variables and output constraints. ` void foo(void) { register int y asm("edx") = 0; asm volatile goto ("mov 42, %0; ja %l1" : "=c"(y) ::: bar); bar: return; } void baz(void) { register int y asm("edx") = 0; asm volatile ("mov 42, %0" : "=c"(y)); } ` The output constraint for asm goto says put the output in %ecx, yet the variable was declared as having register storage in %edx.
   
   Looks like Clang already has bugs or at least disagrees with GCC (for the simpler case of baz, but the problem still exists for foo). Filed https://bugs.llvm.org/show_bug.cgi?id=44328.
   
   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.

Both functions generate the same output for me:

foo:                                    # @foo
	.cfi_startproc
# %bb.0:                                # %entry
	#APP
	movl	42, %edx
	ja	.Ltmp0
	#NO_APP
.Ltmp0:                                 # Block address taken
.LBB0_1:                                # %bar
	retq
.Lfunc_end0:
	.size	foo, .Lfunc_end0-foo
	.cfi_endproc
                                        # -- End function
	.globl	baz                     # -- Begin function baz
	.p2align	4, 0x90
	.type	baz,@function
baz:                                    # @baz
	.cfi_startproc
# %bb.0:                                # %entry
	#APP
	movl	42, %edx
	#NO_APP
	retq

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

[void]

I'm not getting the Undefined temporary symbol in your example (even with optimizations):

[morbo@fawn:llvm-project] cat ../bug.c
void baz(void) {
    int y;
    asm volatile goto ("mov 42, %0; ja %1": "=d"(y) ::: quux);
    asm volatile goto ("mov 42, %0; ja %1": "=c"(y) ::: quux);
    return;
quux:
    return;
}
[morbo@fawn:llvm-project] clang -o - -S ../bug.c
	.text
	.file	"bug.c"
	.globl	baz                     # -- Begin function baz
	.p2align	4, 0x90
	.type	baz,@function
baz:                                    # @baz
	.cfi_startproc
# %bb.0:                                # %entry
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register %rbp
	#APP
	movl	42, %edx
	ja	.Ltmp00
	#NO_APP
	movl	%edx, -8(%rbp)          # 4-byte Spill
	jmp	.LBB0_1
.LBB0_1:                                # %asm.fallthrough
	movl	-8(%rbp), %eax          # 4-byte Reload
	movl	%eax, -4(%rbp)
	#APP
	movl	42, %ecx
	ja	.Ltmp00
	#NO_APP
	movl	%ecx, -12(%rbp)         # 4-byte Spill
	jmp	.LBB0_2
.LBB0_2:                                # %asm.fallthrough1
	movl	-12(%rbp), %eax         # 4-byte Reload
	movl	%eax, -4(%rbp)
	jmp	.LBB0_4
.Ltmp0:                                 # Block address taken
.LBB0_3:                                # %quux
	jmp	.LBB0_4
.LBB0_4:                                # %return
	popq	%rbp
	.cfi_def_cfa %rsp, 8
	retq
.Lfunc_end0:
	.size	baz, .Lfunc_end0-baz
	.cfi_endproc
                                        # -- End function
	.ident	"clang version 10.0.0 (https://github.com/llvm/llvm-project.git 5b71b09a54edbb20900e6d13de35d2592f788330)"
	.section	".note.GNU-stack","",@progbits
	.addrsig
	.addrsig_sym baz

[nickdesaulniers]

In D69876#1791663, @void wrote:

I'm not getting the Undefined temporary symbol in your example (even with optimizations):

[morbo@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, @void wrote:

In 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, @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

clang/test/Sema/asm-goto.cpp

41

what's going on with the indentation?

44

Maybe the formatting changes to the tests (that don't add new tests or meaningfully change existing ones) above can just be pre-committed?

[void]

In D69876#1791989, @nickdesaulniers wrote:

In D69876#1791663, @void wrote:

I'm not getting the Undefined temporary symbol in your example (even with optimizations):

[morbo@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, @void wrote:

In 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, @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

[void]

• Improve the asm goto note in the language extensions doc.

• Include another testcase.

clang/docs/LanguageExtensions.rst

1280

I changed it.

clang/test/Sema/asm-goto.cpp

41

Not sure. I blame emacs. :)

[nickdesaulniers]

clang/test/CodeGen/asm-goto.c

125

Thanks for adding this test. I think it doesn't test that addr is *clobbered* though?

[void]

clang/test/CodeGen/asm-goto.c

125

The + modifier indicates that addr is used for both input and output, in effect being clobbered. Otherwise, we would need to add a "clobber" list, but from what I understand that seems to be restricted to registers, memory, and cc. I wouldn't be able to specify a specific register for addr (e.g. "+D"(addr)) and also have %rdx in the clobber list.

Was there something else you were thinking of?

[nickdesaulniers]

No, thanks for the work on this @void !

[jyknight]

LGTM modulo some minor wording.

clang/docs/LanguageExtensions.rst

1261–1263

I find that sentence confusing. Maybe,
"In addition to the functionality provided by <GCC's extended assembly>, Clang also supports output constraints with the goto form."

clang/lib/AST/Stmt.cpp

645–647

I'm confused about this part. Why isn't the "N" specified in the assembly string already the correct value for the labels? Is the ordering we use internally and that users use externally not the same? I'm assuming your code here is correct, just I'm not understanding, so probably an improved comment would be nice.

[void]

Reword the extension explanation to be more readable.

clang/lib/AST/Stmt.cpp

645–647

The LLVM-specific ordering that I saw was something like:

`<output constraints>,<input constraints>,<plus constraints>,<optional X>,<other constraints>`

The <plus constraints> is empty when there are no output constraints. This just makes up for this. It's probably better to reverse the <optional X> and <plus constraints> parts, but I didn't know how pervasive the order's assumption is in the code.

[void]

Push the correct changes to Phabricator.

[jyknight]

I think you pushed the incorrect change? This was the clang half, but now it's showing the LLVM half. Can you re-upload the diff for the clang half?

[jyknight]

clang/lib/AST/Stmt.cpp

645–647

Oh, right -- in llvm IR, the "plus" constraints don't exist -- we specify those as separate-but-linked output and input constraints.

But thinking about this more...I think this code is actually incorrect. It should be translating the numbers based on where it finds the matching argument-number, not on the 'l' character being present. (And it should do so for named-arguments too).

So, yes, I think it'd be better to just swap the order, and move the plus-operand tied-inputs to the end of the arglist, where they don't affect numbering. I don't _think_ reversing the order will cause other issues.

[jyknight]

Reopening, since this didn't actually land yet. BTW, this review still has the wrong contents in the latest uploaded-diff (showing llvm changes, not clang changes).

[void]

In D69876#1812724, @jyknight wrote:

Reopening, since this didn't actually land yet. BTW, this review still has the wrong contents in the latest uploaded-diff (showing llvm changes, not clang changes).

*Pinches bridge of nose*
*Curses under breath*

clang/lib/AST/Stmt.cpp

645–647

I agree that moving them would make the most sense. Let me do some experiments to see if it breaks anything.

[void]

Move plus constraints to after the label constraints.

[MaskRay]

Does this depend on D69868?

[void]

Fix extension example, found by eagle-eye @MaskRay!

[void]

In D69876#1843199, @MaskRay wrote:

Does this depend on D69868?

Yes. I added it as a parent.

[void]

Merge "__has_extension" patch into this patch.

[void]

Typo

[vvereschaka]

Hello @void ,

this commit breaks Aarch64 builder with failed Clang::uninit-asm-goto.cpp test
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/5020

• FAIL: Clang::uninit-asm-goto.cpp

******************** TEST 'Clang :: Analysis/uninit-asm-goto.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   c:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\build\bin\clang.exe -cc1 -internal-isystem c:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\build\lib\clang\11.0.0\include -nostdsysteminc -std=c++11 -Wuninitialized -verify C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "c:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\build\bin\clang.exe" "-cc1" "-internal-isystem" "c:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\build\lib\clang\11.0.0\include" "-nostdsysteminc" "-std=c++11" "-Wuninitialized" "-verify" "C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp"
# command stderr:
error: 'warning' diagnostics seen but not expected: 

  File C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp Line 6: value size does not match register size specified by the constraint and modifier

  File C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp Line 6: value size does not match register size specified by the constraint and modifier

error: 'note' diagnostics seen but not expected: 

  File C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp Line 6: use constraint modifier "w"

  File C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\clang\test\Analysis\uninit-asm-goto.cpp Line 6: use constraint modifier "w"

4 errors generated.

would you fix the problem or revert the commit?

[leonardchan]

*Bump* We're also seeing the same test fail on our aarch64 bots: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8887516314819686496?blamelist=1#blamelist-tab

FAIL: Clang :: Analysis/uninit-asm-goto.cpp (837 of 17008)
******************** TEST 'Clang :: Analysis/uninit-asm-goto.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/s/w/ir/k/recipe_cleanup/clangu2AOn_/llvm_build_dir/bin/clang -cc1 -internal-isystem /b/s/w/ir/k/recipe_cleanup/clangu2AOn_/llvm_build_dir/lib/clang/11.0.0/include -nostdsysteminc -std=c++11 -Wuninitialized -verify /b/s/w/ir/k/llvm-project/clang/test/Analysis/uninit-asm-goto.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'warning' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/Analysis/uninit-asm-goto.cpp Line 6: value size does not match register size specified by the constraint and modifier
  File /b/s/w/ir/k/llvm-project/clang/test/Analysis/uninit-asm-goto.cpp Line 6: value size does not match register size specified by the constraint and modifier
error: 'note' diagnostics seen but not expected: 
  File /b/s/w/ir/k/llvm-project/clang/test/Analysis/uninit-asm-goto.cpp Line 6: use constraint modifier "w"
  File /b/s/w/ir/k/llvm-project/clang/test/Analysis/uninit-asm-goto.cpp Line 6: use constraint modifier "w"
4 errors generated.

Could you send out a revert or fix soon? Thanks.

[leonardchan]

Woops, sorry. Didn't see you sent out https://reviews.llvm.org/rGe11f9fb4508534d31b09d2ba6cd22428ccc75f65.