r206963 - [ARM64] Change inline assembly constraints to be more lax, to match the behaviour of Clang/AArch64 and GCC.
Richard Smith
richard at metafoo.co.uk
Thu Aug 21 11:06:28 PDT 2014
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?
> 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/20140821/9bfbce44/attachment.html>
More information about the cfe-commits
mailing list