On Thu, Aug 21, 2014 at 11:10 AM, Bob Wilson <[email protected]> wrote:
> > On Aug 21, 2014, at 11:06 AM, Richard Smith <[email protected]> wrote: > > 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? > > > Yes, the “w” modifier change is correct. > Yep. The modifier change is correct. Thanks! -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
