r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
Eric Christopher
echristo at gmail.com
Thu Aug 14 12:00:01 PDT 2014
Couple comments:
+ Pieces.push_back(AsmStringPiece(
+ N, std::string(Begin, CurPtr - Begin),
+ getAsmString()->getLocationOfByte(Percent - StrStart, SM, LO,
TI),
+ getAsmString()->getLocationOfByte(CurPtr - StrStart - 1, SM, LO,
+ TI)));
The strings you're pushing here aren't obvious, putting a comment here and
at the other one would be appreciated as well as a more elaborate comment
here:
+ // Handle %x4 and %x[foo].
Otherwise looks ok to me.
-eric
On Thu Aug 14 2014 at 11:51:34 AM Richard Smith <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");
>
>
> On Thu, Aug 14, 2014 at 8:54 AM, Akira Hatanaka <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>
>> 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>
>>> 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>
>>>> wrote:
>>>>
>>>>> On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <bob.wilson at apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> > On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <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
>>>> 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/20140814/be6432e1/attachment.html>
More information about the cfe-commits
mailing list