r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
Akira Hatanaka
ahatanak at gmail.com
Fri Aug 22 07:22:14 PDT 2014
Committed in r216260.
Thank you all!
On Thu, Aug 21, 2014 at 3:41 PM, Eric Christopher <echristo at gmail.com>
wrote:
>
>
>
> On Thu, Aug 21, 2014 at 11:10 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>>
>> 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>
>> 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 <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <ahatanak at gmail.com>
>>>> wrote:
>>>>
>>>>> Updated patch attached.
>>>>>
>>>>> On Thu, Aug 14, 2014 at 11:51 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");
>>>>>>
>>>>>>
>>>>>>
>>>>> 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>
>>>>>> 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/20140822/c0dd630d/attachment.html>
More information about the cfe-commits
mailing list