Differential D15075
No error for conflict between inputs\outputs and clobber list Authored by • zizhar on Nov 30 2015, 4:36 AM.
Details
According to extended asm syntax, a case where the clobber list includes a variable from the inputs or outputs should be an error - conflict. const long double a = 0.0; 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 Event TimelineComment Actions 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? Comment Actions Given that Clang appears to completely ignore register asm labels on local variables, why should we spend time diagnosing possible conflicts? Comment Actions Changed the test to use global variables. Comment Actions 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. Comment Actions 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. 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: So I believe the check is in place and the test above can be added as a good test case. Comment Actions 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. Comment Actions 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?
Comment Actions 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. Comment Actions
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).
Isn't a) equivalent to b)? a) b) 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. Comment Actions A redo, investigated a bit more, had to fix some things, moved it to ActOnGCCAsmStmt.
Comment Actions Hello Hans, Thanks, Comment Actions Akira, My new solution is going to be to create another virtual function in TargetInfo that will do everything I need. Thanks,
Comment Actions 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.
Comment Actions Hi, Comment Actions 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. Comment Actions Hello :) Ziv Comment Actions 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.
Comment Actions Changed the '^' to point to the clobber which conflicts. Comment Actions You should use git-clang-format or some equivalent to format your change.
Comment Actions I have a couple of minor comments, but patch looks good to me.
Comment Actions 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. Thanks, Comment Actions 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. Comment Actions 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. Comment Actions 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). Comment Actions Vitaly, Test was fixed by in Review: Comment Actions 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? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I don't think things specific to x86 should appear in SemaStmtAsm.cpp. Also, is it possible to use X86TargetInfo::convertConstraint?