On Fri, Aug 15, 2014 at 2:52 PM, Akira Hatanaka <[email protected]> wrote:
> Updated patch is attached. I made the following changes since I sent the > last patch: > > - Added comments in GCCAsmStmt::AnalyzeAsmString to make it clearer what > the string and the range represent. > - Fixed typo "modifer" in note_asm_modifier's string. > - Added test case in which '%' comes from a macro. If I use macros, clang > doesn't seem to highlight the fixit range or print the string, but that is > true for other kinds of warnings too. > > #define PERCENT "%" > > ^ (no fixit string or range here) > > 3 warnings generated. > > RIchard, is this the "interesting" test case you were expecting? > Yes, thank you! Please add a CHECK-NOT: fix-it after your CHECK:s for this case. Other than that, this looks good to me, but someone else will need to confirm that the fix to add the 'w' modifier is correct; Eric? > On Thu, Aug 14, 2014 at 2:09 PM, Richard Smith <[email protected]> > wrote: > >> 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. >>> >>> >> I would like a test where the source locations of different parts of the >> string literal are "interesting"; for instance, where the '%' and the >> following characters come from different parts of the source code. >> >> >>> 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
