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 >> >> >
f1.c:8:48: warning: value size does not match register size specified by the
constraint and modifier [-Wasm-operand-widths]
__asm__ volatile("ldr%%\nb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
^
f1.c:8:30: note: Use constraint modifer "w"
__asm__ volatile("ldr%%\nb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
^~
%w0
f1.c:19:55: warning: value size does not match register size specified by the
constraint and modifier [-Wasm-operand-widths]
__asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) :
"memory");
^
f1.c:19:26: note: Use constraint modifer "w"
__asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) :
"memory");
^~~~~
%w[s0]
2 warnings generated.
inline-asm-modifier3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
