r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
Bob Wilson
bob.wilson at apple.com
Thu Aug 21 11:10:19 PDT 2014
> On Aug 21, 2014, at 11:06 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Fri, Aug 15, 2014 at 2:52 PM, Akira Hatanaka <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> 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.
>
> On Thu, Aug 14, 2014 at 2:09 PM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
> On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> wrote:
> Updated patch attached.
>
> On Thu, Aug 14, 2014 at 11:51 AM, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> 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 <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> 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 <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> 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 <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> 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 <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> wrote:
> On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <bob.wilson at apple.com <mailto:bob.wilson at apple.com>> wrote:
>
> > On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <ahatanak at gmail.com <mailto:ahatanak at gmail.com>> 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
> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits <http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140821/3bce187b/attachment.html>
More information about the cfe-commits
mailing list