<div dir="ltr">Thanks,<div>I will change the patch accordingly and commit it later on.</div><div><br></div><div>Alex.</div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-05-28 16:34 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-May-28, at 15:52, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi dexonsmith, bob.wilson, bogner,<br>
><br>
> This patch translates the line and column numbers from LLVM IR block string to the<br>
> line and column numbers in the MIR file so that llc reports LLVM IR errors with<br>
> correct locations.<br>
><br>
> REPOSITORY<br>
> rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10108&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=mdcLkxGuizqKpL203AESHtsBAX5rLhrKDt4qLOZXgeQ&s=23oK0yQFD4iCc_DWyzEvfPLsK3cwc2xBsTewlu2hGeo&e=" target="_blank">http://reviews.llvm.org/D10108</a><br>
><br>
> Files:<br>
> lib/CodeGen/MIRParser/MIRParser.cpp<br>
> test/CodeGen/MIR/llvm-ir-error-reported.mir<br>
><br>
> Index: lib/CodeGen/MIRParser/MIRParser.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/MIRParser/MIRParser.cpp<br>
> +++ lib/CodeGen/MIRParser/MIRParser.cpp<br>
> @@ -18,6 +18,7 @@<br>
> #include "llvm/AsmParser/Parser.h"<br>
> #include "llvm/CodeGen/MIRYamlMapping.h"<br>
> #include "llvm/IR/Module.h"<br>
> +#include "llvm/Support/LineIterator.h"<br>
> #include "llvm/Support/SMLoc.h"<br>
> #include "llvm/Support/SourceMgr.h"<br>
> #include "llvm/Support/MemoryBuffer.h"<br>
> @@ -49,6 +50,11 @@<br>
> ///<br>
> /// Return true if an error occurred.<br>
> bool parseMachineFunction(yaml::Input &In);<br>
> +<br>
> +private:<br>
> + /// Return a MIR diagnostic converted from an LLVM assembly diagnostic.<br>
> + SMDiagnostic diagFromLLVMAssemblyDiag(const SMDiagnostic &Error,<br>
> + SMRange SourceRange);<br>
> };<br>
><br>
> } // end anonymous namespace<br>
> @@ -81,8 +87,10 @@<br>
> dyn_cast_or_null<yaml::BlockScalarNode>(In.getCurrentNode())) {<br>
> M = parseAssembly(MemoryBufferRef(BSN->getValue(), Filename), Error,<br>
> Context);<br>
> - if (!M)<br>
> + if (!M) {<br>
> + Error = diagFromLLVMAssemblyDiag(Error, BSN->getSourceRange());<br>
> return M;<br>
> + }<br>
> In.nextDocument();<br>
> if (!In.setCurrentDocument())<br>
> return M;<br>
> @@ -111,6 +119,36 @@<br>
> return false;<br>
> }<br>
><br>
> +SMDiagnostic MIRParserImpl::diagFromLLVMAssemblyDiag(const SMDiagnostic &Error,<br>
> + SMRange SourceRange) {<br>
> + assert(SourceRange.isValid());<br>
> +<br>
> + // Translate the location of the error from the location in the llvm IR string<br>
> + // to the corresponding location in the MIR file.<br>
> + auto LineAndColumn = SM.getLineAndColumn(SourceRange.Start);<br>
> + unsigned Line = LineAndColumn.first + Error.getLineNo() - 1;<br>
> + unsigned Column = Error.getColumnNo();<br>
> + StringRef LineStr = Error.getLineContents();<br>
> + SMLoc Loc = Error.getLoc();<br>
> +<br>
> + // Get the full line and adjust the column number by taking the indentation of<br>
> + // LLVM IR into account.<br>
> + line_iterator Lines(*SM.getMemoryBuffer(SM.getMainFileID()), false);<br>
> + while (!Lines.is_at_end() && Lines.line_number() != Line)<br>
> + ++Lines;<br>
> + if (Lines.line_number() == Line) {<br>
> + LineStr = *Lines;<br>
> + Loc = SMLoc::getFromPointer(LineStr.data());<br>
> + auto Indent = LineStr.find(Error.getLineContents());<br>
> + if (Indent != StringRef::npos)<br>
> + Column += Indent;<br>
> + }<br>
<br>
</div></div>According to the `line_iterator` docs, calling `line_number()` isn't<br>
safe at EOF. Your `if` statement looks like it could violate that.<br>
<br>
I think you should change this to use a standard iterator pattern, and<br>
only use the iterator when it's valid:<br>
<br>
for (line_iterator L(*SM.getMemoryBuffer(SM.getMainFileID()), false), E;<br>
L != E; ++L)<br>
if (L.line_number() == Line) {<br>
LineStr = *L;<br>
Loc = SMLoc::getFromPointer(LineStr.data());<br>
auto Indent = LineStr.find(Error.getLineContents());<br>
<span class=""> if (Indent != StringRef::npos)<br>
</span> Column += Indent;<br>
break;<br>
}<br>
<br>
With that change, this LGTM.<br>
<div class="HOEnZb"><div class="h5"><br>
> +<br>
> + return SMDiagnostic(SM, Loc, Filename, Line, Column, Error.getKind(),<br>
> + Error.getMessage(), LineStr, Error.getRanges(),<br>
> + Error.getFixIts());<br>
> +}<br>
> +<br>
> std::unique_ptr<Module> llvm::parseMIRFile(StringRef Filename,<br>
> SMDiagnostic &Error,<br>
> LLVMContext &Context) {<br>
> Index: test/CodeGen/MIR/llvm-ir-error-reported.mir<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ test/CodeGen/MIR/llvm-ir-error-reported.mir<br>
> @@ -0,0 +1,22 @@<br>
> +# RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s<br>
> +# This test ensures an error is reported if the embedded LLVM IR contains an<br>
> +# error.<br>
> +<br>
> +--- |<br>
> +<br>
> + ; CHECK: [[@LINE+3]]:15: error: use of undefined value '%a'<br>
> + define i32 @foo(i32 %x, i32 %y) {<br>
> + %z = alloca i32, align 4<br>
> + store i32 %a, i32* %z, align 4<br>
> + br label %Test<br>
> + Test:<br>
> + %m = load i32, i32* %z, align 4<br>
> + %cond = icmp eq i32 %y, %m<br>
> + br i1 %cond, label %IfEqual, label %IfUnequal<br>
> + IfEqual:<br>
> + ret i32 1<br>
> + IfUnequal:<br>
> + ret i32 0<br>
> + }<br>
> +<br>
> +...<br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div>