I noticed there is a typo in the diagnostic message of note_asm_missing_constraint_modifier. "modifer" should be "modifier". I'll fix that too.
On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <[email protected]> wrote: > Updated patch attached. > > On Thu, Aug 14, 2014 at 11:51 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"); >> >> >> > I didn't understand what this piece of code is testing. Could you > elaborate on it? When I compile it with '-fsyntax-only', clang complains > that there is a missing closing parenthesis. > > *testmacro1.c:6:32: **error: **expected ')'* > > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > > * ^* > > *testmacro1.c:6:8: note: *to match this '(' > > __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : > "memory"); > > * ^* > > 1 error generated. > > > > >> 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
