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
Thu Aug 14 08:54:17 PDT 2014
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/16d0c5f8/attachment.html>
-------------- next part --------------
f1.c:8:48: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
__asm__ volatile("ldr%%\nb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
^
f1.c:8:30: note: Use constraint modifer "w"
__asm__ volatile("ldr%%\nb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");
^~
%w0
f1.c:19:55: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
__asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");
^
f1.c:19:26: note: Use constraint modifer "w"
__asm__ volatile("ldrb %[s0], [%[s1]]" : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");
^~~~~
%w[s0]
2 warnings generated.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inline-asm-modifier3.patch
Type: application/octet-stream
Size: 10637 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140814/16d0c5f8/attachment.obj>
More information about the cfe-commits
mailing list