No error for conflict between inputs\outputs and clobber list

[• zizhar]

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.

[ahatanak]

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]

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

[• zizhar]

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]

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.

[myatsina]

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]

In D15075#309678, @myatsina wrote:

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]

Fixed in the desired location

[• zizhar]

Is this ok? :)

[ahatanak]

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

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

[• zizhar]

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.

[ahatanak]

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]

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]

include/clang/Basic/TargetInfo.h

585

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

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

[• zizhar]

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

[• zizhar]

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]

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?

[ahatanak]

In D15075#486986, @zizhar wrote:

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]

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?

[ahatanak]

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.

[• zizhar]

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

Ziv

[ahatanak]

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

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

lib/Basic/TargetInfo.cpp

406

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

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

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]

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]

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

include/clang/Basic/TargetInfo.h

597

format

600

format

lib/Basic/TargetInfo.cpp

405

format

lib/Sema/SemaStmtAsm.cpp

144

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

153

format

185

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

[ahatanak]

lib/Basic/TargetInfo.cpp

430

Please use braces or a ternary operator here.

lib/Basic/Targets.cpp

2714

Use "override" instead of "virtual".

2718

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

lib/Sema/SemaStmtAsm.cpp

186

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

Return "SourceLocation()"

599

You don't need braces here.

[• zizhar]

fixed :)

[ahatanak]

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

lib/Sema/SemaStmtAsm.cpp

141

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

151

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

[• zizhar]

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

[ahatanak]

LGTM with a few nits.

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

lib/Sema/SemaStmtAsm.cpp

142

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

160

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

[• zizhar]

changed :)
Thanks for the review :>

[vitalybuka]

These patches break asan tests: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio

[myatsina]

In D15075#631207, @vitalybuka wrote:

These patches break asan tests: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio

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

[ahatanak]

In D15075#631237, @myatsina wrote:

In D15075#631207, @vitalybuka wrote:

These patches break asan tests: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio

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.

[myatsina]

In D15075#631316, @ahatanak wrote:

In D15075#631237, @myatsina wrote:

In D15075#631207, @vitalybuka wrote:

These patches break asan tests: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/528/steps/check-asan%20in%20gcc%20build/logs/stdio

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.

[ahatanak]

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

[myatsina]

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]

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?

[myatsina]

In D15075#940711, @dolson wrote:

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.

include/clang/Basic/DiagnosticSemaKinds.td

     "constraint '%0' is already present here">;
 }
 
+  def error_inoutput_conflict_with_clobber : Error<
+    "asm-specifier for input or output variable conflicts with asm"
+    " clobber list">;
+
 let CategoryName = "Semantic Issue" in {
 
 def err_invalid_conversion_between_vectors : Error<

include/clang/Basic/TargetInfo.h

   /// \brief Returns the "normalized" GCC register name.
   ///
   /// For example, on x86 it will return "ax" when "eax" is passed in.
-  StringRef getNormalizedGCCRegisterName(StringRef Name) const;
+  StringRef getNormalizedGCCRegisterName(StringRef Name,
+    bool ReturnCannonical = false) const;
 
+  virtual StringRef getConstraintRegister(const StringRef &Constraint,
+    const StringRef &Expression) const {
+    return "";
+  }
+
   struct ConstraintInfo {
     enum {
       CI_None = 0x00,

lib/Basic/TargetInfo.cpp

   return false;
 }
 
+// ReturnCannonical true will return the register name without any additions
+// such as "{}" or "%"
 StringRef
-TargetInfo::getNormalizedGCCRegisterName(StringRef Name) const {
+TargetInfo::getNormalizedGCCRegisterName(StringRef Name,
+bool ReturnCannonical) const {
   assert(isValidGCCRegisterName(Name) && "Invalid register passed in");
 
   // Get rid of any register prefix.

       // Make sure the register that the additional name is for is within
       // the bounds of the register names from above.
       if (AN == Name && ARN.RegNum < Names.size())
-        return Name;
+        if (ReturnCannonical)
+          return Names[ARN.RegNum];
+        else
+          return Name;
     }
 
   // Now check aliases.

lib/Basic/Targets.cpp

   const char *getClobbers() const override {
     return "~{dirflag},~{fpsr},~{flags}";
   }
+  virtual StringRef getConstraintRegister(const StringRef &Constraint,
+    const StringRef &Expression) const {
+    for (int i = 0; i < Constraint.size(); ++i) {
+      switch (Constraint[i]) {
+        // Ignore these
+      case '*':
+      case '?':
+      case '!':
+        // Will see this and the following in mult-alt constraints.
+      case '=':
+      case '+':
+        continue;
+        // For the register constraints, return the matching register name
+      case 'a':
+        return "ax";
+      case 'b':
+        return "bx";
+      case 'c':
+        return "cx";
+      case 'd':
+        return "dx";
+      case 'S':
+        return "si";
+      case 'D':
+        return "di";
+        // In case the constraint is 'r' we need to extract the register name
+      case 'r':
+        return Expression;
+      default:
+        // Default value if there is no constraint for the register
+        return "";
+      }
+    }
+    return "";
+  }
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
   static void setSSELevel(llvm::StringMap<bool> &Features, X86SSEEnum Level,

lib/Headers/intrin.h

 #if defined(__i386__) || defined(__x86_64__)
 static __inline__ void __DEFAULT_FN_ATTRS
 __movsb(unsigned char *__dst, unsigned char const *__src, size_t __n) {
-  __asm__("rep movsb" : : "D"(__dst), "S"(__src), "c"(__n)
-                        : "%edi", "%esi", "%ecx");
+  __asm__("rep movsb" : : "D"(__dst), "S"(__src), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __movsd(unsigned long *__dst, unsigned long const *__src, size_t __n) {
-  __asm__("rep movsl" : : "D"(__dst), "S"(__src), "c"(__n)
-                        : "%edi", "%esi", "%ecx");
+  __asm__("rep movsl" : : "D"(__dst), "S"(__src), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __movsw(unsigned short *__dst, unsigned short const *__src, size_t __n) {
-  __asm__("rep movsw" : : "D"(__dst), "S"(__src), "c"(__n)
-                        : "%edi", "%esi", "%ecx");
+  __asm__("rep movsw" : : "D"(__dst), "S"(__src), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __stosb(unsigned char *__dst, unsigned char __x, size_t __n) {
-  __asm__("rep stosb" : : "D"(__dst), "a"(__x), "c"(__n)
-                        : "%edi", "%ecx");
+  __asm__("rep stosb" : : "D"(__dst), "a"(__x), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __stosd(unsigned long *__dst, unsigned long __x, size_t __n) {
-  __asm__("rep stosl" : : "D"(__dst), "a"(__x), "c"(__n)
-                        : "%edi", "%ecx");
+  __asm__("rep stosl" : : "D"(__dst), "a"(__x), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __stosw(unsigned short *__dst, unsigned short __x, size_t __n) {
-  __asm__("rep stosw" : : "D"(__dst), "a"(__x), "c"(__n)
-                        : "%edi", "%ecx");
+  __asm__("rep stosw" : : "D"(__dst), "a"(__x), "c"(__n));
 }
 #endif
 #ifdef __x86_64__
 static __inline__ void __DEFAULT_FN_ATTRS
 __movsq(unsigned long long *__dst, unsigned long long const *__src, size_t __n) {
-  __asm__("rep movsq" : : "D"(__dst), "S"(__src), "c"(__n)
-                        : "%edi", "%esi", "%ecx");
+  __asm__("rep movsq" : : "D"(__dst), "S"(__src), "c"(__n));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __stosq(unsigned __int64 *__dst, unsigned __int64 __x, size_t __n) {
-  __asm__("rep stosq" : : "D"(__dst), "a"(__x), "c"(__n)
-                        : "%edi", "%ecx");
+  __asm__("rep stosq" : "=D"(__dst) : "a"(__x), "c"(__n));
 }
 #endif
 

lib/Sema/SemaStmtAsm.cpp

 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/MC/MCParser/MCAsmParser.h"
 using namespace clang;
 using namespace sema;

   return false;
 }
 
+// Extracting the register name from the Expression value,
+// if there is no register name to extract, returns ""
+StringRef ExtractRegisterName(const Expr *Expression, const TargetInfo &Target) {
+  while (const ImplicitCastExpr *P = dyn_cast<ImplicitCastExpr>(Expression)) {
+    Expression = P->getSubExpr();
+  }
+  if (const DeclRefExpr *AsmDeclRef = dyn_cast<DeclRefExpr>(Expression)) {
+    // Handle cases where the expression is a variable
+    const VarDecl *Variable = dyn_cast<VarDecl>(AsmDeclRef->getDecl());
+    if (Variable && Variable->getStorageClass() == SC_Register) {
+      if (AsmLabelAttr *Attr = Variable->getAttr<AsmLabelAttr>())
+        return Target.isValidGCCRegisterName(Attr->getLabel())
+        ? Target.getNormalizedGCCRegisterName(Attr->getLabel(), true)
+        : "";
+    }
+  }
+  return "";
+}
+
+// Checks if there is a conflict between the input and output lists with the
+// clobbers list
+bool HasClobberConflict(MultiExprArg Exprs, StringLiteral **Constraints,
+  StringLiteral **Clobbers, int NumClobbers,
+  const TargetInfo &Target, ASTContext &Cont) {
+  llvm::StringSet<> InOutVars;
+  // Collect all the input and output registers from the extended asm statement
+  // in order to check for conflicts with the clobber list
+  for (int i = 0; i < Exprs.size(); ++i) {
+    StringRef Constraint = Constraints[i]->getString();
+    StringRef InOutReg = Target.getConstraintRegister(
+      Constraint, ExtractRegisterName(Exprs[i], Target));
+    if (InOutReg != "")
+      InOutVars.insert(InOutReg);
+  }
+  // Check for each item in the clobber list if it conflicts with the input
+  // or output
+  for (int i = 0; i < NumClobbers; ++i) {
+    StringRef Clobber = Clobbers[i]->getString();
+    // We only check registers, therefore we don't check cc and memory clobbers
+    if (Clobber == "cc" || Clobber == "memory")
+      continue;
+    Clobber = Target.getNormalizedGCCRegisterName(Clobber, true);
+    // Go over the output's registers we collected
+    if (InOutVars.find(Clobber) != InOutVars.end())
+      return true;
+  }
+  return false;
+}
+
 StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                                  bool IsVolatile, unsigned NumOutputs,
                                  unsigned NumInputs, IdentifierInfo **Names,

     return StmtError();
   }
 
+  // Check for conflicts between clobber list and input or output lists
+  if (HasClobberConflict(Exprs, Constraints, Clobbers, NumClobbers,
+    Context.getTargetInfo(), Context)) {
+    return Diag(NS->getAsmLoc(),
+      diag::error_inoutput_conflict_with_clobber);
+  }
   return NS;
 }
 

test/Sema/asm.c

   asm ("nop" : : : "204"); // expected-error {{unknown register name '204' in asm}}
   asm ("nop" : : : "-1"); // expected-error {{unknown register name '-1' in asm}}
   asm ("nop" : : : "+1"); // expected-error {{unknown register name '+1' in asm}}
+  register void *clobber_conflict asm ("%rcx");
+  register void *no_clobber_conflict asm ("%rax");
+  int a,b,c;
+  asm ("nop" : "=r" (no_clobber_conflict) : "r" (clobber_conflict) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=r" (clobber_conflict) : "r" (no_clobber_conflict) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=r" (clobber_conflict) : "r" (clobber_conflict) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=c" (a) : "r" (no_clobber_conflict) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=r" (no_clobber_conflict) : "c" (c) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=r" (clobber_conflict) : "c" (c) : "%rcx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}}
+  asm ("nop" : "=a" (a) : "b" (b) : "%rcx", "%rbx"); // expected-error {{asm-specifier for input or output variable conflicts with asm clobber list}} 
 }
 
 // rdar://6094010