From: Akira Hatanaka Date: Fri, 22 Aug 2014 06:05:21 +0000 (+0000) Subject: [AArch64, inline-asm] Improve diagnostic that is printed when the size of a X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=dfc0ac7f3dc3f309bdc48135622d02d9c2efbd56;p=clang [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@216260 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h index 790c8e3e66..d42f491056 100644 --- a/include/clang/AST/Stmt.h +++ b/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 diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 52381b9334..4cdc6c084b 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/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; + + def note_asm_missing_constraint_modifier : Note< + "use constraint modifier \"%0\"">; } let CategoryName = "Semantic Issue" in { diff --git a/include/clang/Basic/TargetInfo.h b/include/clang/Basic/TargetInfo.h index edef7c0377..e26f595c3d 100644 --- a/include/clang/Basic/TargetInfo.h +++ b/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, diff --git a/lib/AST/Stmt.cpp b/lib/AST/Stmt.cpp index d12642ddfc..c80e0ef067 100644 --- a/lib/AST/Stmt.cpp +++ b/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&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&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&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; diff --git a/lib/Basic/Targets.cpp b/lib/Basic/Targets.cpp index d1eba5af47..2e65589722 100644 --- a/lib/Basic/Targets.cpp +++ b/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; } } } diff --git a/lib/Sema/SemaStmtAsm.cpp b/lib/Sema/SemaStmtAsm.cpp index 5d076cac94..47a7672ae1 100644 --- a/lib/Sema/SemaStmtAsm.cpp +++ b/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. diff --git a/test/Sema/arm64-inline-asm.c b/test/Sema/arm64-inline-asm.c index 2cfbe46929..d8e16a6872 100644 --- a/test/Sema/arm64-inline-asm.c +++ b/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"}} } diff --git a/test/Sema/inline-asm-validate-aarch64.c b/test/Sema/inline-asm-validate-aarch64.c new file mode 100644 index 0000000000..1364b6421e --- /dev/null +++ b/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; +} diff --git a/test/Sema/inline-asm-validate.c b/test/Sema/inline-asm-validate.c index 6fa760c809..73335e76cd 100644 --- a/test/Sema/inline-asm-validate.c +++ b/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; }