Committed in r216260. Thank you all!
On Thu, Aug 21, 2014 at 3:41 PM, Eric Christopher <[email protected]> wrote: > > > > 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
