<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 21, 2014 at 11:10 AM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class=""><blockquote type="cite"><div>On Aug 21, 2014, at 11:06 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>
<br><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 15, 2014 at 2:52 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Updated patch is attached. I made the following changes since I sent the last patch:<div><br></div><div>
- Added comments in GCCAsmStmt::AnalyzeAsmString to make it clearer what the string and the range represent.</div>
<div>- Fixed typo "modifer" in note_asm_modifier's string.<br></div><div><div>- 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.</div>
<div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo">#define PERCENT "%"</div><div style="margin:0px;font-size:11px;font-family:Menlo"> ^ (no fixit string or range here)</div>
<div style="margin:0px;font-size:11px;font-family:Menlo">3 warnings generated.</div></div><div><br></div></div><div><div>RIchard, is this the "interesting" test case you were expecting?</div></div></div></blockquote>
<div><br></div><div>Yes, thank you!</div><div><br></div><div>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?</div>
</div></div></div></div></blockquote><div><br></div></div>Yes, the “w” modifier change is correct.</div></div></blockquote><div><br></div><div>Yep. The modifier change is correct.</div><div><br></div><div>Thanks!</div><div>
<br></div><div>-eric </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><div><br><blockquote type="cite"><div>
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 14, 2014 at 2:09 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On Thu, Aug 14, 2014 at 1:46 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Updated patch attached.<br><div class="gmail_extra"><br><div class="gmail_quote"><div>On Thu, Aug 14, 2014 at 11:51 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Thanks!</div><div><br></div><div>+ SourceRange getRange() const {<br>
</div><div><br></div><div>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.</div>
<div><br></div><div>+ def note_asm_missing_constraint_modifier : Note<</div><div>+ "Use constraint modifer \"%0\"">;</div><div><br></div><div>Diagnostics should start with a lowercase letter.</div>
<div><br></div><div>Please add a testcase for weirder ways of writing the string literal; things like</div><div><br></div><div> uint8_t byte, *addr;</div><div> #define constraint(x) "%" #x</div><div> #define mem(x) constraint([x])</div>
<div> #define ldrb(x, y) "ldrb " x ", " y</div><div> __asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</div></div><div><div>
<div class="gmail_extra">
<br><br></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div><br></div><div><div style="margin:0px;font-size:11px"><font face="courier new, monospace"><b>testmacro1.c:6:32: </b><span style="color:rgb(195,55,32)"><b>error: </b></span><b>expected ')'</b></font></div><div>
<div style="margin:0px;font-size:11px"><font face="courier new, monospace">__asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</font></div>
</div><div style="margin:0px;font-size:11px;color:rgb(52,189,38)"><b><font face="courier new, monospace"> ^</font></b></div><div style="margin:0px;font-size:11px"><font face="courier new, monospace"><b>testmacro1.c:6:8: note: </b>to match this '('</font></div>
<div><div style="margin:0px;font-size:11px"><font face="courier new, monospace">__asm__(ldrb(mem(s0), mem(s1)) : [s0] "=r" (byte) : [s1] "r" (addr) : "memory");</font></div>
</div><div style="margin:0px;font-size:11px;color:rgb(52,189,38)"><b><font face="courier new, monospace"> ^</font></b></div><div style="margin:0px;font-size:11px;font-family:Menlo">1 error generated.</div><p style="margin:0px;font-size:11px;font-family:Menlo">
<span style="color:rgb(80,0,80);font-family:arial;font-size:small"></span></p></div></div>
</div></div></blockquote><div><br></div></div><div>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.</div>
<div><div>
<div><span style="color:rgb(80,0,80)"> </span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 14, 2014 at 8:54 AM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">Updated patch which addresses comments from Eric and Richard is attached. Also, you can see the diagnostic messages clang currently prints in diag1.txt.<div><div><div><br></div><div class="gmail_extra">
<div class="gmail_quote">
On Wed, Aug 13, 2014 at 11:38 AM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>This looks completely wrong:</div><div>
<br></div>+ LocStart.getLocWithOffset(DiagOffs)));<br>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra"><div class="gmail_extra">+ B.AddFixItHint(FixItHint::CreateInsertion(Loc.getLocWithOffset(-1),</div><div class="gmail_extra">+ SuggestedModifier));</div>
<div><br></div><div>Likewise here.</div><div><br></div><div>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.</div>
<div><br></div><div><div>+ __asm__ volatile("ldrb %0, [%1]" : "=r" (byte) : "r" (addr) : "memory");</div><div>+// CHECK: warning: value size does not match register size specified by the constraint and modifier</div>
<div>+// CHECK:{{^}} ^</div><div>+// CHECK:{{^}} "%w0"</div></div><div><br></div><div>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.</div>
</div><br><div class="gmail_quote"><div><div>On Tue, Aug 12, 2014 at 5:56 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
<div dir="ltr">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.</div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">
On Tue, Aug 12, 2014 at 4:51 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div>On Tue, Aug 12, 2014 at 4:13 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br></div></div><div class="gmail_extra">
<div class="gmail_quote"><div><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><br>
> On Aug 11, 2014, at 2:42 PM, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>> wrote:<br>
><br>
> The attached patch improves upon the patch James sent out a few weeks ago.<br>
><br>
> 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.<br>
><br>
<br>
</div>Some comments/questions….<br>
<br>
> diff --git a/include/clang/AST/Stmt.h b/include/clang/AST/Stmt.h<br>
> index 790c8e3..9ce6d22 100644<br>
> --- a/include/clang/AST/Stmt.h<br>
> +++ b/include/clang/AST/Stmt.h<br>
> @@ -1580,18 +1580,17 @@ public:<br>
> Kind MyKind;<br>
> std::string Str;<br>
> unsigned OperandNo;<br>
> + SourceLocation Loc;<br>
> public:<br>
> AsmStringPiece(const std::string &S) : MyKind(String), Str(S) {}<br>
> - AsmStringPiece(unsigned OpNo, char Modifier)<br>
> - : MyKind(Operand), Str(), OperandNo(OpNo) {<br>
> - Str += Modifier;<br>
> + AsmStringPiece(unsigned OpNo, const std::string &S, SourceLocation L)<br>
> + : MyKind(Operand), Str(S), OperandNo(OpNo), Loc(L) {<br>
> }<br>
><br>
> bool isString() const { return MyKind == String; }<br>
> bool isOperand() const { return MyKind == Operand; }<br>
><br>
> const std::string &getString() const {<br>
> - assert(isString());<br>
> return Str;<br>
> }<br>
><br>
<br>
Why did you remove the isString() assert?<br>
<br></blockquote><div><br></div></div></div><div>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".</div>
<div>
<div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> diff --git a/include/clang/Basic/Diagnostic.h b/include/clang/Basic/Diagnostic.\<br>
> h<br>
> index a7b2ba2..4d53ade 100644<br>
> --- a/include/clang/Basic/Diagnostic.h<br>
> +++ b/include/clang/Basic/Diagnostic.h<br>
> @@ -773,6 +773,10 @@ private:<br>
> /// or modify at a particular position.<br>
> SmallVector<FixItHint, 8> DiagFixItHints;<br>
><br>
> + /// \brief Caret location. This is set only when the caret location is<br>
> + /// different from the current diagnostic location in CurDiagLoc.<br>
> + SourceLocation CaretLoc;<br>
> +<br>
> DiagnosticMapping makeUserMapping(diag::Severity Map, SourceLocation L) {<br>
> bool isPragma = L.isValid();<br>
> DiagnosticMapping Mapping =<br>
><br>
<br>
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.<br>
<br></blockquote><div><br></div></div><div>I didn't think of using the existing location, but that sounds like a better solution.</div><div><div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
> diff --git a/include/clang/Frontend/DiagnosticRenderer.h b/include/clang/Fronte\<br>
> nd/DiagnosticRenderer.h<br>
> index ce1dc90..1a4cae4 100644<br>
> --- a/include/clang/Frontend/DiagnosticRenderer.h<br>
> +++ b/include/clang/Frontend/DiagnosticRenderer.h<br>
> @@ -88,7 +88,8 @@ protected:<br>
> DiagnosticsEngine::Level Level,<br>
> SmallVectorImpl<CharSourceRange>& Ranges,<br>
> ArrayRef<FixItHint> Hints,<br>
> - const SourceManager &SM) = 0;<br>
> + const SourceManager &SM,<br>
> + SourceLocation CaretLoc) = 0;<br>
><br>
> virtual void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,<br>
> const SourceManager &SM) = 0;<br>
> @@ -116,7 +117,7 @@ private:<br>
> void emitModuleBuildStack(const SourceManager &SM);<br>
> void emitCaret(SourceLocation Loc, DiagnosticsEngine::Level Level,<br>
> ArrayRef<CharSourceRange> Ranges, ArrayRef<FixItHint> Hints,<br>
> - const SourceManager &SM);<br>
> + const SourceManager &SM, SourceLocation CaretLoc);<br>
> void emitMacroExpansions(SourceLocation Loc,<br>
> DiagnosticsEngine::Level Level,<br>
> ArrayRef<CharSourceRange> Ranges,<br>
> @@ -132,17 +133,19 @@ public:<br>
> /// information needed based on macros whose expansions impact the<br>
> /// diagnostic.<br>
> ///<br>
> - /// \param Loc The location for this caret.<br>
> + /// \param Error location.<br>
><br>
<br>
You dropped the parameter name here.</blockquote></div></div></div><br></div><div class="gmail_extra">I'll fix this and send a new patch.</div><div class="gmail_extra"><br></div></div>
</blockquote></div><br></div>
</div></div><br></div></div><div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>
</div></blockquote></div><br></div></div></div></blockquote></div><br></div></div>