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 14 14:09:49 PDT 2014


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/20140814/995d6eee/attachment.html>


More information about the cfe-commits mailing list