[AArch64, inline-asm] Improve diagnostic that is printed when the size of a

variable that has regiser constraint "r" is not 64-bit.

General register operands are output using 64-bit "x" register names, regardless
of the size of the variable, unless the asm operand is prefixed with the "%w"
modifier. This surprises and confuses many users who aren't familiar with
aarch64 inline assembly rules.

With this commit, a note and fixit hint are printed which tell the users that
they need modifier "%w" in order to output a "w" register instead of an "x"
register.

<rdar://problem/12764785>

llvm-svn: 216260

clang/include/clang/AST/Stmt.h

@@ -1580,18 +1580,21 @@ public:
     Kind MyKind;
     std::string Str;
     unsigned OperandNo;
+
+    // Source range for operand references.
+    CharSourceRange Range;
   public:
     AsmStringPiece(const std::string &S) : MyKind(String), Str(S) {}
-    AsmStringPiece(unsigned OpNo, char Modifier)
-      : MyKind(Operand), Str(), OperandNo(OpNo) {
-      Str += Modifier;
+    AsmStringPiece(unsigned OpNo, const std::string &S, SourceLocation Begin,
+                   SourceLocation End)
+      : MyKind(Operand), Str(S), OperandNo(OpNo),
+        Range(CharSourceRange::getCharRange(Begin, End)) {
     }
 
     bool isString() const { return MyKind == String; }
     bool isOperand() const { return MyKind == Operand; }
 
     const std::string &getString() const {
-      assert(isString());
       return Str;
     }
 
@@ -1600,12 +1603,14 @@ public:
       return OperandNo;
     }
 
+    CharSourceRange getRange() const {
+      assert(isOperand() && "Range is currently used only for Operands.");
+      return Range;
+    }
+
     /// getModifier - Get the modifier for this operand, if present.  This
     /// returns '\0' if there was no modifier.
-    char getModifier() const {
-      assert(isOperand());
-      return Str[0];
-    }
+    char getModifier() const;
   };
 
   /// AnalyzeAsmString - Analyze the asm string of the current asm, decomposing

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -6038,6 +6038,9 @@ let CategoryName = "Inline Assembly Issue" in {
     "value size does not match register size specified by the constraint "
     "and modifier">,
     InGroup<ASMOperandWidths>;
+
+  def note_asm_missing_constraint_modifier : Note<
+    "use constraint modifier \"%0\"">;
 }
 
 let CategoryName = "Semantic Issue" in {

clang/include/clang/Basic/TargetInfo.h

@@ -581,9 +581,11 @@ public:
                                  unsigned /*Size*/) const {
     return true;
   }
-  virtual bool validateConstraintModifier(StringRef /*Constraint*/,
-                                          const char /*Modifier*/,
-                                          unsigned /*Size*/) const {
+  virtual bool
+  validateConstraintModifier(StringRef /*Constraint*/,
+                             char /*Modifier*/,
+                             unsigned /*Size*/,
+                             std::string &/*SuggestedModifier*/) const {
     return true;
   }
   bool resolveSymbolicName(const char *&Name,

clang/lib/AST/Stmt.cpp

@@ -357,6 +357,11 @@ unsigned AsmStmt::getNumPlusOperands() const {
   return Res;
 }
 
+char GCCAsmStmt::AsmStringPiece::getModifier() const {
+  assert(isOperand() && "Only Operands can have modifiers.");
+  return isLetter(Str[0]) ? Str[0] : '\0';
+}
+
 StringRef GCCAsmStmt::getClobber(unsigned i) const {
   return getClobberStringLiteral(i)->getString();
 }
@@ -517,17 +522,25 @@ unsigned GCCAsmStmt::AnalyzeAsmString(SmallVectorImpl<AsmStringPiece>&Pieces,
       CurStringPiece.clear();
     }
 
-    // Handle %x4 and %x[foo] by capturing x as the modifier character.
-    char Modifier = '\0';
+    // Handle operands that have asmSymbolicName (e.g., %x[foo]) and those that
+    // don't (e.g., %x4). 'x' following the '%' is the constraint modifier.
+
+    const char *Begin = CurPtr - 1; // Points to the character following '%'.
+    const char *Percent = Begin - 1; // Points to '%'.
+
     if (isLetter(EscapedChar)) {
       if (CurPtr == StrEnd) { // Premature end.
         DiagOffs = CurPtr-StrStart-1;
         return diag::err_asm_invalid_escape;
       }
-      Modifier = EscapedChar;
       EscapedChar = *CurPtr++;
     }
 
+    const TargetInfo &TI = C.getTargetInfo();
+    const SourceManager &SM = C.getSourceManager();
+    const LangOptions &LO = C.getLangOpts();
+
+    // Handle operands that don't have asmSymbolicName (e.g., %x4).
     if (isDigit(EscapedChar)) {
       // %n - Assembler operand n
       unsigned N = 0;
@@ -543,11 +556,21 @@ unsigned GCCAsmStmt::AnalyzeAsmString(SmallVectorImpl<AsmStringPiece>&Pieces,
         return diag::err_asm_invalid_operand_number;
       }
 
-      Pieces.push_back(AsmStringPiece(N, Modifier));
+      // Str contains "x4" (Operand without the leading %).
+      std::string Str(Begin, CurPtr - Begin);
+
+      // (BeginLoc, EndLoc) represents the range of the operand we are currently
+      // processing. Unlike Str, the range includes the leading '%'.
+      SourceLocation BeginLoc =
+          getAsmString()->getLocationOfByte(Percent - StrStart, SM, LO, TI);
+      SourceLocation EndLoc =
+          getAsmString()->getLocationOfByte(CurPtr - StrStart, SM, LO, TI);
+
+      Pieces.push_back(AsmStringPiece(N, Str, BeginLoc, EndLoc));
       continue;
     }
 
-    // Handle %[foo], a symbolic operand reference.
+    // Handle operands that have asmSymbolicName (e.g., %x[foo]).
     if (EscapedChar == '[') {
       DiagOffs = CurPtr-StrStart-1;
 
@@ -566,7 +589,18 @@ unsigned GCCAsmStmt::AnalyzeAsmString(SmallVectorImpl<AsmStringPiece>&Pieces,
         DiagOffs = CurPtr-StrStart;
         return diag::err_asm_unknown_symbolic_operand_name;
       }
-      Pieces.push_back(AsmStringPiece(N, Modifier));
+
+      // Str contains "x[foo]" (Operand without the leading %).
+      std::string Str(Begin, NameEnd + 1 - Begin);
+
+      // (BeginLoc, EndLoc) represents the range of the operand we are currently
+      // processing. Unlike Str, the range includes the leading '%'.
+      SourceLocation BeginLoc =
+          getAsmString()->getLocationOfByte(Percent - StrStart, SM, LO, TI);
+      SourceLocation EndLoc =
+          getAsmString()->getLocationOfByte(NameEnd + 1 - StrStart, SM, LO, TI);
+
+      Pieces.push_back(AsmStringPiece(N, Str, BeginLoc, EndLoc));
 
       CurPtr = NameEnd+1;
       continue;

clang/lib/Basic/Targets.cpp

@@ -4145,8 +4145,10 @@ public:
     }
     return R;
   }
-  bool validateConstraintModifier(StringRef Constraint, const char Modifier,
-                                  unsigned Size) const override {
+  bool
+  validateConstraintModifier(StringRef Constraint, const char Modifier,
+                             unsigned Size,
+                             std::string &SuggestedModifier) const override {
     bool isOutput = (Constraint[0] == '=');
     bool isInOut = (Constraint[0] == '+');
 
@@ -4592,9 +4594,10 @@ public:
     return false;
   }
 
-  virtual bool validateConstraintModifier(StringRef Constraint,
-                                          const char Modifier,
-                                          unsigned Size) const {
+  bool
+  validateConstraintModifier(StringRef Constraint, const char Modifier,
+                             unsigned Size,
+                             std::string &SuggestedModifier) const override {
     // Strip off constraint modifiers.
     while (Constraint[0] == '=' || Constraint[0] == '+' || Constraint[0] == '&')
       Constraint = Constraint.substr(1);
@@ -4613,7 +4616,11 @@ public:
       default:
         // By default an 'r' constraint will be in the 'x'
         // registers.
-        return Size == 64;
+        if (Size == 64)
+          return true;
+
+        SuggestedModifier = "w";
+        return false;
       }
     }
     }

clang/lib/Sema/SemaStmtAsm.cpp

@@ -257,11 +257,22 @@ StmtResult Sema::ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
       continue;
 
     unsigned Size = Context.getTypeSize(Ty);
-    if (!Context.getTargetInfo()
-          .validateConstraintModifier(Literal->getString(), Piece.getModifier(),
-                                      Size))
+    std::string SuggestedModifier;
+    if (!Context.getTargetInfo().validateConstraintModifier(
+            Literal->getString(), Piece.getModifier(), Size,
+            SuggestedModifier)) {
       Diag(Exprs[ConstraintIdx]->getLocStart(),
            diag::warn_asm_mismatched_size_modifier);
+
+      if (!SuggestedModifier.empty()) {
+        auto B = Diag(Piece.getRange().getBegin(),
+                      diag::note_asm_missing_constraint_modifier)
+                 << SuggestedModifier;
+        SuggestedModifier = "%" + SuggestedModifier + Piece.getString();
+        B.AddFixItHint(FixItHint::CreateReplacement(Piece.getRange(),
+                                                    SuggestedModifier));
+      }
+    }
   }
 
   // Validate tied input operands for type mismatches.

clang/test/Sema/arm64-inline-asm.c

@@ -5,5 +5,5 @@ void foo() {
   asm volatile("USE(%x0)" :: "z"(0LL));
   asm volatile("USE(%w0)" :: "z"(0));
 
-  asm volatile("USE(%0)" :: "z"(0)); // expected-warning {{value size does not match register size specified by the constraint and modifier}}
+  asm volatile("USE(%0)" :: "z"(0)); // expected-warning {{value size does not match register size specified by the constraint and modifier}} expected-note {{use constraint modifier "w"}}
 }

clang/test/Sema/inline-asm-validate-aarch64.c

@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -triple arm64-apple-darwin -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+typedef unsigned char uint8_t;
+
+uint8_t constraint_r(uint8_t *addr) {
+  uint8_t byte;
+
+  __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
+// CHECK: warning: value size does not match register size specified by the constraint and modifier
+// CHECK: note: use constraint modifier "w"
+// CHECK: fix-it:{{.*}}:{8:26-8:28}:"%w0"
+
+  return byte;
+}
+
+uint8_t constraint_r_symbolic(uint8_t *addr) {
+  uint8_t byte;
+
+  __asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");
+// CHECK: warning: value size does not match register size specified by the constraint and modifier
+// CHECK: note: use constraint modifier "w"
+// CHECK: fix-it:{{.*}}:{19:26-19:31}:"%w[s0]"
+
+  return byte;
+}
+
+#define PERCENT "%"
+
+uint8_t constraint_r_symbolic_macro(uint8_t *addr) {
+  uint8_t byte;
+
+  __asm__ volatile("ldrb "PERCENT"[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");
+// CHECK: warning: value size does not match register size specified by the constraint and modifier
+// CHECK: note: use constraint modifier "w"
+// CHECK-NOT: fix-it
+
+  return byte;
+}

clang/test/Sema/inline-asm-validate.c

@@ -3,6 +3,6 @@
 unsigned t, r, *p;
 
 int foo (void) {
-  __asm__ __volatile__( "stxr   %w[_t], %[_r], [%[_p]]" : [_t] "=&r" (t) : [_p] "p" (p), [_r] "r" (r) : "memory"); // expected-warning{{value size does not match register size specified by the constraint and modifier}}
+  __asm__ __volatile__( "stxr   %w[_t], %[_r], [%[_p]]" : [_t] "=&r" (t) : [_p] "p" (p), [_r] "r" (r) : "memory"); // expected-warning{{value size does not match register size specified by the constraint and modifier}} expected-note {{use constraint modifier "w"}}
   return 1;
 }