Page MenuHomePhabricator

No error for conflict between inputs\outputs and clobber list
ClosedPublic

Authored by zizhar on Nov 30 2015, 4:36 AM.

Details

Summary

According to extended asm syntax, a case where the clobber list includes a variable from the inputs or outputs should be an error - conflict.
for example:

const long double a = 0.0;
int main()
{

char b;
double t1 = a;
__asm__ ("fucompp": "=a" (b) : "u" (t1), "t" (t1) : "cc", "st", "st(1)");

return 0;
}

This should conflict with the output - t1 which is st, and st which is st aswell

The patch fixes it.

Diff Detail

Repository
rL LLVM

Event Timeline

zizhar updated this revision to Diff 41384.Nov 30 2015, 4:36 AM
zizhar retitled this revision from to No error for conflict between inputs\outputs and clobber list.
zizhar updated this object.
zizhar added reviewers: rnk, ahatanak.
zizhar set the repository for this revision to rL LLVM.
ahatanak edited edge metadata.Nov 30 2015, 2:23 PM

gcc detects the case in which the register in the clobber list overlaps with the register associated with the local variable but isn't identical to it. For example, it still errors out if the clobbered register is %ax instead of %eax. Do you have plans to have clang detect this case in follow-up patches?

rnk requested changes to this revision.Nov 30 2015, 5:43 PM
rnk edited edge metadata.

Given that Clang appears to completely ignore register asm labels on local variables, why should we spend time diagnosing possible conflicts?

This revision now requires changes to proceed.Nov 30 2015, 5:43 PM
zizhar updated this object.Dec 10 2015, 6:05 AM
zizhar edited edge metadata.

Changed the test to use global variables.
We can also use "esp" as register asm label and the problem exists.
The patch fixes it.

rnk added a comment.Dec 11 2015, 2:01 PM

I don't see the test case in the patch.

I also don't really see the value behind this diagnostic in general. The only register variables that Clang/LLVM support are pre-allocated registers that are outside the normal allocatable set. Why does it matter if the user lists them in their clobber set? They are not general purpose registers, and the compiler doesn't normally use them for register allocation.

I've revisited the register asm labels on local variables, and it seems that clang does not ignore them, at least when there's inline asm involved - if I change "%rcx" to "%rax" it indeed changes the generated asm code.
This is the example I used:

void * foo (void *p)
{

register void *q asm ("%rcx") = p;
asm ("mov %0, %%rbx" : "=r" (q) : "0" (q) : "%rcx"); 
return q;

}

gcc produces the following error for such conflicts:
error: asm-specifier for variable "q" conflicts with asm clobber list
clang, on the other hand, compiles successfully.

So I believe the check is in place and the test above can be added as a good test case.

rnk added a comment.Dec 14 2015, 1:55 PM

I've revisited the register asm labels on local variables, and it seems that clang does not ignore them, at least when there's inline asm involved - if I change "%rcx" to "%rax" it indeed changes the generated asm code.

Ah, you're right, I see how it works. Sorry about that!

I would prefer it if we did this validation in Sema instead of CodeGen. See Sema::ActOnGCCAsmStmt for all the other validation code. You should be able to build a set of all the normalized register names of register asm label variables and then check the normalized clobber name for set membership.

Also consider using range-based for and StringRef when appropriate.

zizhar edited edge metadata.
zizhar removed rL LLVM as the repository for this revision.

Fixed in the desired location

Is this ok? :)

It seems to me that the purpose of this patch (based on what I see in the summary of this review) is to make clang error out when the following code is compiled.

int foo1(int t2) {

int c;
__asm__ __volatile__ ("asm1 %0, %1": "=a" (c) : "b" (t2) : "cc", "ebx"); // "b" and "ebx" overlap.
return c;

}

But clang doesn't issue any warnings or errors while gcc does ("error: 'asm' operand has impossible constraints"). Is there a reason clang doesn't detect this case?

test/Sema/asm.c
40 ↗(On Diff #43822)

What does "r" ("%rcx") do?

I'm not familiar with this, but it Is this something explained here?

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm

You understood corectly :)

After going a bit through the log, I think that there is no reason for clang to not detect it, probably the check was just forgotten.

This patch is the check.

"r" constraint means (taken from the spec:) A register operand is allowed provided that it is in a general register.
("%rcx") means - use the register rcx.

After going a bit through the log, I think that there is no reason for clang to not detect it, probably the check was just forgotten.

This patch is the check.

Which part of your patch (in SemaStmtAsm.cpp) is supposed to catch that? I applied this patch and rebuilt clang, but it still doesn't error out when it compiles the function in my example (foo1).

"r" constraint means (taken from the spec:) A register operand is allowed provided that it is in a general register.
("%rcx") means - use the register rcx.

Isn't a) equivalent to b)?

a)
asm volatile ("asm1 %0, %1": "=a" (c) : "r" ("%ebx") : "cc", "ebx"); // do "b" and "%ebx" overlap?

b)
const char *s = "%ebx";
asm volatile ("asm1 %0, %1": "=a" (c) : "r" (s) : "cc", "ebx");

gcc and clang generate the same code for these two statements. I believe constraint "r" means clang can use any general purpose registers for the operand.

zizhar updated this revision to Diff 60996.EditedJun 16 2016, 11:07 AM
zizhar edited edge metadata.

A redo, investigated a bit more, had to fix some things, moved it to ActOnGCCAsmStmt.
Fixed intrin.h (the patch reveals a problem in it).
Added tests to check for regression and examples.

ahatanak added inline comments.Jul 1 2016, 6:33 PM
include/clang/Basic/TargetInfo.h
585 ↗(On Diff #60996)

This gives build errors. TargetInfo:: isn't needed here.

lib/Headers/Intrin.h
843 ↗(On Diff #60996)

If this is an unrelated bug that you discovered while working on this patch, you should probably send a separate patch to the list and contact the author.

lib/Sema/SemaStmtAsm.cpp
179 ↗(On Diff #60996)

I don't think things specific to x86 should appear in SemaStmtAsm.cpp. Also, is it possible to use X86TargetInfo::convertConstraint?

Hello Hans,
Regarding the changes in intrin.h,
You're the last to change/add these lines, I found out that there is a conflict between the clobber list and input or output lists.
Can you review this change?

Thanks,
Ziv Izhar

Akira,
You've mentioned a good point, this X86 logic should indeed be moved to X86TargetInfo.
The current convertConstraint() implementation is not doing what I need – it doesn’t handle all the input/output constraints I need, and it returns the constraint in a different format than the one I need.
E.g. convertConstraint() does not handle “r” constraints or special characters like “=”, “+”, also, the function returns the registers as “{ax}”, when I need the “ax”.
I think it is better to add a new function for my logic instead of trying to adjust this function to handle both its old logic and my new logic.

My new solution is going to be to create another virtual function in TargetInfo that will do everything I need.
The code that is currently under GetConstraintRegister() and ExtractRegisterName() will now be part of the X86TargetInfo implementation of this virtual function.
For all the other architectures’ TargetInfos I’ll create a default implementation that will return an empty string and they can implement it if they want to (until they do, the existing behavior will be retained).
Can I have your opinion on this? Do you see a better way to implement this?

Thanks,
Ziv Izhar.

majnemer added inline comments.
lib/Headers/Intrin.h
841–874 ↗(On Diff #60996)

The clobber should be "memory", no?
Er, and shouldn't the we list __dst, __x and __n as outputs because they are modified?

Akira,
You've mentioned a good point, this X86 logic should indeed be moved to X86TargetInfo.
The current convertConstraint() implementation is not doing what I need – it doesn’t handle all the input/output constraints I need, and it returns the constraint in a different format than the one I need.
E.g. convertConstraint() does not handle “r” constraints or special characters like “=”, “+”, also, the function returns the registers as “{ax}”, when I need the “ax”.
I think it is better to add a new function for my logic instead of trying to adjust this function to handle both its old logic and my new logic.

My new solution is going to be to create another virtual function in TargetInfo that will do everything I need.
The code that is currently under GetConstraintRegister() and ExtractRegisterName() will now be part of the X86TargetInfo implementation of this virtual function.
For all the other architectures’ TargetInfos I’ll create a default implementation that will return an empty string and they can implement it if they want to (until they do, the existing behavior will be retained).
Can I have your opinion on this? Do you see a better way to implement this?

I agree with you. You can define a virtual function in TargetInfo, which checks conflicts between clobbers and input/output, and override it in X86's TargetInfo.

lib/Headers/Intrin.h
841–874 ↗(On Diff #60996)

Yes, I think we need "memory" here because it writes to the memory pointed to by dst.

Do we need to add x to the output too? I think dst and n are modfied by stosq and rep (which I think is why %edi and %ecx were added to the clobber list), but I don't see how x gets modified.

zizhar updated this revision to Diff 70660.Sep 8 2016, 6:55 AM

Hi,
I have moved the relevant code to TargetInfo as previously discussed, and created a default implementation and the x86 implementation.
Regarding the intrinsics, I believe it should be a separate fix, as it is not related to the clobber conflict this review is about.
Do you want me to open a bugzilla on the intrinsics?

You can create a separate patch for the changes made to lib/Headers/intrin.h and have it reviewed before committing this patch.

Also, dst, x and __n should be added to the output list and "memory" to the clobber list as majnemer pointed out. I think you can use constraint "+" to ensure the registers are clobbered. For example:

__asm__("rep movsb" : "+D"(__dst), "+S"(__src), "+c"(__n) : : "memory");

For stos, "a"(__x) can remain an input constraint since it doesn't get modified.

Hello :)
I will prepare another patch as you suggested,
is this patch good?
Can it be submitted?

Ziv

This patch doesn't detect more complex conflicts (like the following code), but I think it's OK for now.

asm ("foo" : "=Q" (x) : : "%rax", "%rbx", "%rcx", "%rdx"); // Q: Any register accessible as rh: a, b, c, and d.
include/clang/Basic/TargetInfo.h
593 ↗(On Diff #70660)

It looks like this comment was wrong: it didn't return "ax" when "eax" was passed in Name.

lib/Basic/TargetInfo.cpp
406 ↗(On Diff #70660)

I think it's spelled "Canonical" rather than "Cannonical".

The comment added here doesn't seem correct to me. ReturnCanonical is used to make this function return the "canonical" register instead of the register passed in Name (e.g., when ReturnCanonical=true and Name="rax", it returns "ax").

lib/Basic/Targets.cpp
2718 ↗(On Diff #70660)

Can we skip the checks for all these non-alphabet characters and handle them in "default"? Would doing so be incorrect?

test/Sema/asm.c
34 ↗(On Diff #70660)

Do you think you can improve the error message using '^' so that it's clear which constraint has a conflict with the clobbers? I was thinking about something like this:

error: inline-asm constraint "=r" conflicts with clobber list

asm ("nop" : "=r" (no_clobber_conflict) : "r" (clobber_conflict) : "%rcx");

^
zizhar updated this revision to Diff 73617.Oct 5 2016, 3:59 AM

Changed the '^' to point to the clobber which conflicts.
Changed the relevant comments.
However, regarding the point about the non-alphabetical characters - It's a different behavior for these specific characters,
since these should be ignored and the default case is for an unknown character.

rnk added a comment.Oct 5 2016, 1:54 PM

You should use git-clang-format or some equivalent to format your change.

include/clang/Basic/TargetInfo.h
597 ↗(On Diff #73617)

format

600 ↗(On Diff #73617)

format

lib/Basic/TargetInfo.cpp
405 ↗(On Diff #73617)

format

lib/Sema/SemaStmtAsm.cpp
144 ↗(On Diff #73617)

This is a reimplementation of Expression->IgnoreImpCasts(), use that instead.

153 ↗(On Diff #73617)

format

185 ↗(On Diff #73617)

This can be InOutVars.count(Clobber), which is more idiomatic for testing set membership.

ahatanak added inline comments.Oct 5 2016, 10:40 PM
lib/Basic/TargetInfo.cpp
430 ↗(On Diff #73617)

Please use braces or a ternary operator here.

lib/Basic/Targets.cpp
2718 ↗(On Diff #70660)

Sorry for not being clear. Because this will fail to find registers for constraints like "=&c", I was thinking you can just check the first alphabet after skipping all the non-alphabets (using isalpha, for example).

2714 ↗(On Diff #73617)

Use "override" instead of "virtual".

lib/Sema/SemaStmtAsm.cpp
186 ↗(On Diff #73617)

You don't want to return the address of a local variable. Just change this function to return a SourceLocation by value. You can use SourceLocation::isValid in ActOnGCCAsmStmt to determine whether it is a valid SourceLocation.

190 ↗(On Diff #73617)

Return "SourceLocation()"

602 ↗(On Diff #73617)

You don't need braces here.

I have a couple of minor comments, but patch looks good to me.

lib/Sema/SemaStmtAsm.cpp
141 ↗(On Diff #73760)

Please remove all trailing spaces and "^M" characters in this patch. Also, run clang-format and fix indentations.

151 ↗(On Diff #73760)

You can just return getNormalizedGCCRegisterName() if Target.isValidGCCRegisterName() is true here.

zizhar updated this revision to Diff 74068.Oct 9 2016, 4:35 AM

I hope this last update finishes it,
Thanks for the review :)

ahatanak accepted this revision.Oct 10 2016, 1:26 PM
ahatanak edited edge metadata.

LGTM with a few nits.

Please fix lib/Headers/intrin.h too and commit it as a separate patch.

lib/Sema/SemaStmtAsm.cpp
142 ↗(On Diff #74068)

This should be a static function that starts with a lower case letter.

160 ↗(On Diff #74068)

This should be a static function that starts with a lower case letter.

zizhar updated this revision to Diff 74500.Oct 13 2016, 5:19 AM
zizhar edited edge metadata.

changed :)
Thanks for the review :>

This revision was automatically updated to reflect the committed changes.

Vitaly ,

This patch is a gcc compatibility issue and it changes clang to output an error in case of illegal inline assembly syntax related to clobber list.

Commit r290540 changed an asan test (asan_asm_test.cc) that used the illegal syntax and fixed it.
The commit removed from the extended inline assembly clobber list registers that also appeared in the input list.
GCC fails as well on the original inline assembly that appeared in this test, so the fix is correct.
I don't understand why this change has effect the logic of the test - can you help?

Thanks,
Marina

Vitaly ,

This patch is a gcc compatibility issue and it changes clang to output an error in case of illegal inline assembly syntax related to clobber list.

Commit r290540 changed an asan test (asan_asm_test.cc) that used the illegal syntax and fixed it.
The commit removed from the extended inline assembly clobber list registers that also appeared in the input list.
GCC fails as well on the original inline assembly that appeared in this test, so the fix is correct.
I don't understand why this change has effect the logic of the test - can you help?

Thanks,
Marina

I believe asm_rep_movs needs something in the output operand list that tells the compiler the inline-asm statement changes the contents of the registers ("S", "D" and "c"). Otherwise, the compiler (register allocator) will not save the old value of dst_good and src_good so that it can be used later in the static_assert:

asm_rep_movs(dst_good, src_good, 4);
ASSERT_EQ(static_cast<T>(0x0), dst_good[0]); // "D" register was incremented 4 times.

Vitaly ,

This patch is a gcc compatibility issue and it changes clang to output an error in case of illegal inline assembly syntax related to clobber list.

Commit r290540 changed an asan test (asan_asm_test.cc) that used the illegal syntax and fixed it.
The commit removed from the extended inline assembly clobber list registers that also appeared in the input list.
GCC fails as well on the original inline assembly that appeared in this test, so the fix is correct.
I don't understand why this change has effect the logic of the test - can you help?

Thanks,
Marina

I believe asm_rep_movs needs something in the output operand list that tells the compiler the inline-asm statement changes the contents of the registers ("S", "D" and "c"). Otherwise, the compiler (register allocator) will not save the old value of dst_good and src_good so that it can be used later in the static_assert:

asm_rep_movs(dst_good, src_good, 4);
ASSERT_EQ(static_cast<T>(0x0), dst_good[0]); // "D" register was incremented 4 times.

You are right, the "S" and "D" constraint are wrong as well in both DECLARE_ASM_REP_MOVS macros, the right assembly should probably be something like:

asm("rep movsb \n\t"

: "D"(dst), "S"(src)
: "S"(src), "c"(size)
: "memory");

The different movs instructions change both esi/rsi and edi/rdi (movs copies esi to edi and then also increments/decrements both registers), so esi/rdi is both input and output while edi/rdi is just output.

The problem is I am not able to build the AsanSanitizer unit test and check if this fixes the issue.

I guess it doesn't build because output constraints need "=" (e.g., "=D")?

Also, I think all registers ("D", "S", and "c") should be in both the output and input operands list. You can probably declare new variables and use them in the output operands (e.g., "=D"(newDst)) or use input/output operands "+" (the former is simpler in this case, since you want to use the original values of dst and src).

Vitaly,
Thanks for fixing the test!

Test was fixed by in
rL290621: [asan] Fix test broken by r290540

Review:
https://reviews.llvm.org/D28128

dolson added a subscriber: dolson.Nov 30 2017, 10:21 AM

Hello,

In the process of upgrading from clang 3.6.1 to a newer version, I ran into this new error and thus imported the new intrinsics from intrin.h for rep movsb and friends. I see several discussions in this thread about how having the registers solely in the inputs list is not sufficient for something like "rep movsb" because the modified registers will not be clobbered, however none of these suggested changes made it into the eventual intrin.h.

I found that using the versions of __movsb and __stosb that are at the head revision intrin.h produced bad code generation vs the versions with the clobbers. Note this is on PS4 under the older clang 3.6.1, but I don't see anything in this CL that would update the clobber behavior for newer versions of clang.

Shouldn't the intrinsics be updated to use input/output registers or some other method of clobbering?

Hello,

In the process of upgrading from clang 3.6.1 to a newer version, I ran into this new error and thus imported the new intrinsics from intrin.h for rep movsb and friends. I see several discussions in this thread about how having the registers solely in the inputs list is not sufficient for something like "rep movsb" because the modified registers will not be clobbered, however none of these suggested changes made it into the eventual intrin.h.

I found that using the versions of __movsb and __stosb that are at the head revision intrin.h produced bad code generation vs the versions with the clobbers. Note this is on PS4 under the older clang 3.6.1, but I don't see anything in this CL that would update the clobber behavior for newer versions of clang.

Shouldn't the intrinsics be updated to use input/output registers or some other method of clobbering?

Reid created https://reviews.llvm.org/D40686 which addresses this issue.