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 15 14:52:49 PDT 2014
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?
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/20140815/0890dc97/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inline-asm-modifier5.patch
Type: application/octet-stream
Size: 12010 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140815/0890dc97/attachment.obj>
More information about the cfe-commits
mailing list