Couple comments: + Pieces.push_back(AsmStringPiece( + N, std::string(Begin, CurPtr - Begin), + getAsmString()->getLocationOfByte(Percent - StrStart, SM, LO, TI), + getAsmString()->getLocationOfByte(CurPtr - StrStart - 1, SM, LO, + TI)));
The strings you're pushing here aren't obvious, putting a comment here and at the other one would be appreciated as well as a more elaborate comment here: + // Handle %x4 and %x[foo]. Otherwise looks ok to me. -eric On Thu Aug 14 2014 at 11:51:34 AM Richard Smith <[email protected]> wrote: > Thanks! > > + SourceRange getRange() const { > > This should be a CharSourceRange; SourceRange is a token range. You're > getting away with this because running the lexer within the string literal > happens to skip to the end of the modifier, but there are almost certainly > ways to observe this problem. > > + def note_asm_missing_constraint_modifier : Note< > + "Use constraint modifer \"%0\"">; > > Diagnostics should start with a lowercase letter. > > Please add a testcase for weirder ways of writing the string literal; > things like > > uint8_t byte, *addr; > #define constraint(x) "%" #x > #define mem(x) constraint([x]) > #define ldrb(x, y) "ldrb " x ", " y > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > > > On Thu, Aug 14, 2014 at 8:54 AM, Akira Hatanaka <[email protected]> > wrote: > >> Updated patch which addresses comments from Eric and Richard is attached. >> Also, you can see the diagnostic messages clang currently prints in >> diag1.txt. >> >> On Wed, Aug 13, 2014 at 11:38 AM, Richard Smith <[email protected]> >> wrote: >> >>> This looks completely wrong: >>> >>> + LocStart.getLocWithOffset( >>> DiagOffs))); >>> >>> As far as I can see, DiagOffs is an offset within the string, not an >>> offset within the source file. This will do the wrong thing if the string >>> literal is even slightly interesting (if there are character escapes, >>> escaped newlines, trigraphs, string literal concatenation, macros, ...). >>> Instead you should use StringLiteral::getLocationOfByte to find the >>> SourceLocation of a particular character in the string literal. >>> >>> + B.AddFixItHint(FixItHint::CreateInsertion(Loc. >>> getLocWithOffset(-1), >>> + SuggestedModifier)); >>> >>> Likewise here. >>> >>> Also, this is adding a fix-it hint to a warning. That's only permitted >>> if applying the fix-it hint would not change the semantics of the code >>> (because fix-its on errors get automatically applied in some cases, and in >>> order to support this, it's important that we recover as if all fix-its on >>> errors were applied, and warnings are sometimes upgraded to errors). >>> Otherwise, the fix-it should be on a separate note. >>> >>> + __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:{{^}} ^ >>> +// CHECK:{{^}} "%w0" >>> >>> Please check your fixit with -fdiagnostics-parseable-fixits rather than >>> by FileChecking the user-facing output. See the existing tests in >>> test/FixIt for examples. >>> >>> On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <[email protected]> >>> wrote: >>> >>>> Updated patch is attached. As per Bob's suggestion, I changed it to use >>>> the existing diagnostic location instead of defining a separate location >>>> for the caret. >>>> >>>> >>>> On Tue, Aug 12, 2014 at 4:51 PM, Akira Hatanaka <[email protected]> >>>> wrote: >>>> >>>>> On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> > On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <[email protected]> >>>>>> wrote: >>>>>> > >>>>>> > The attached patch improves upon the patch James sent out a few >>>>>> weeks ago. >>>>>> > >>>>>> > It changes validateConstraintModifier to return the suggested >>>>>> modifier and also makes changes to print the caret and the suggested >>>>>> modifier right below the operand in the assembly template. >>>>>> > >>>>>> >>>>>> Some comments/questions…. >>>>>> >>>>>> > diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h >>>>>> > index 790c8e3..9ce6d22 100644 >>>>>> > --- a/include/clang/AST/Stmt.h >>>>>> > +++ b/include/clang/AST/Stmt.h >>>>>> > @@ -1580,18 +1580,17 @@ public: >>>>>> > Kind MyKind; >>>>>> > std::string Str; >>>>>> > unsigned OperandNo; >>>>>> > + SourceLocation Loc; >>>>>> > 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 L) >>>>>> > + : MyKind(Operand), Str(S), OperandNo(OpNo), Loc(L) { >>>>>> > } >>>>>> > >>>>>> > bool isString() const { return MyKind == String; } >>>>>> > bool isOperand() const { return MyKind == Operand; } >>>>>> > >>>>>> > const std::string &getString() const { >>>>>> > - assert(isString()); >>>>>> > return Str; >>>>>> > } >>>>>> > >>>>>> >>>>>> Why did you remove the isString() assert? >>>>>> >>>>>> >>>>> When GCCAsmStmt::AnalyzeAsmString parses the "%0" or "%[s0]", it >>>>> constructs an AsmStringPiece that is an Operand (i.e., MyKind is >>>>> "Operand", >>>>> not "String"). Later when Sema::ActOnGCCAsmStmt calls getString to create >>>>> the fixit string, it asserts because it isn't a "String". >>>>> >>>>> >>>>>> > diff --git a/include/clang/Basic/Diagnostic.h >>>>>> b/include/clang/Basic/Diagnostic.\ >>>>>> > h >>>>>> > index a7b2ba2..4d53ade 100644 >>>>>> > --- a/include/clang/Basic/Diagnostic.h >>>>>> > +++ b/include/clang/Basic/Diagnostic.h >>>>>> > @@ -773,6 +773,10 @@ private: >>>>>> > /// or modify at a particular position. >>>>>> > SmallVector<FixItHint, 8> DiagFixItHints; >>>>>> > >>>>>> > + /// \brief Caret location. This is set only when the caret >>>>>> location is >>>>>> > + /// different from the current diagnostic location in CurDiagLoc. >>>>>> > + SourceLocation CaretLoc; >>>>>> > + >>>>>> > DiagnosticMapping makeUserMapping(diag::Severity Map, >>>>>> SourceLocation L) { >>>>>> > bool isPragma = L.isValid(); >>>>>> > DiagnosticMapping Mapping = >>>>>> > >>>>>> >>>>>> Can you explain more why you need a separate SourceLocation for this? >>>>>> i.e., Why not just set the existing diagnostic location? It seems like a >>>>>> large part of this patch is related to handling this separate caret >>>>>> location, and I’d like to understand why that is needed. >>>>>> >>>>>> >>>>> I didn't think of using the existing location, but that sounds like a >>>>> better solution. >>>>> >>>>> >>>>>> > diff --git a/include/clang/Frontend/DiagnosticRenderer.h >>>>>> b/include/clang/Fronte\ >>>>>> > nd/DiagnosticRenderer.h >>>>>> > index ce1dc90..1a4cae4 100644 >>>>>> > --- a/include/clang/Frontend/DiagnosticRenderer.h >>>>>> > +++ b/include/clang/Frontend/DiagnosticRenderer.h >>>>>> > @@ -88,7 +88,8 @@ protected: >>>>>> > DiagnosticsEngine::Level Level, >>>>>> > SmallVectorImpl<CharSourceRange>& >>>>>> Ranges, >>>>>> > ArrayRef<FixItHint> Hints, >>>>>> > - const SourceManager &SM) = 0; >>>>>> > + const SourceManager &SM, >>>>>> > + SourceLocation CaretLoc) = 0; >>>>>> > >>>>>> > virtual void emitIncludeLocation(SourceLocation Loc, >>>>>> PresumedLoc PLoc, >>>>>> > const SourceManager &SM) = 0; >>>>>> > @@ -116,7 +117,7 @@ private: >>>>>> > void emitModuleBuildStack(const SourceManager &SM); >>>>>> > void emitCaret(SourceLocation Loc, DiagnosticsEngine::Level >>>>>> Level, >>>>>> > ArrayRef<CharSourceRange> Ranges, >>>>>> ArrayRef<FixItHint> Hints, >>>>>> > - const SourceManager &SM); >>>>>> > + const SourceManager &SM, SourceLocation CaretLoc); >>>>>> > void emitMacroExpansions(SourceLocation Loc, >>>>>> > DiagnosticsEngine::Level Level, >>>>>> > ArrayRef<CharSourceRange> Ranges, >>>>>> > @@ -132,17 +133,19 @@ public: >>>>>> > /// information needed based on macros whose expansions impact >>>>>> the >>>>>> > /// diagnostic. >>>>>> > /// >>>>>> > - /// \param Loc The location for this caret. >>>>>> > + /// \param Error location. >>>>>> > >>>>>> >>>>>> You dropped the parameter name here. >>>>> >>>>> >>>>> I'll fix this and send a new patch. >>>>> >>>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
