[PATCH] MIR Serialization: Report diagnostics with correct locations for LLVM IR Errors.

[Duncan P. N. Exon Smith]

> On 2015-May-28, at 15:52, Alex Lorenz <arphaman at gmail.com> wrote:
> 
> Hi dexonsmith, bob.wilson, bogner,
> 
> This patch translates the line and column numbers from LLVM IR block string to the
> line and column numbers in the MIR file so that llc reports LLVM IR errors with
> correct locations.
> 
> REPOSITORY
>  rL LLVM
> 
> http://reviews.llvm.org/D10108
> 
> Files:
>  lib/CodeGen/MIRParser/MIRParser.cpp
>  test/CodeGen/MIR/llvm-ir-error-reported.mir
> 
> Index: lib/CodeGen/MIRParser/MIRParser.cpp
> ===================================================================
> --- lib/CodeGen/MIRParser/MIRParser.cpp
> +++ lib/CodeGen/MIRParser/MIRParser.cpp
> @@ -18,6 +18,7 @@
>  #include "llvm/AsmParser/Parser.h"
>  #include "llvm/CodeGen/MIRYamlMapping.h"
>  #include "llvm/IR/Module.h"
> +#include "llvm/Support/LineIterator.h"
>  #include "llvm/Support/SMLoc.h"
>  #include "llvm/Support/SourceMgr.h"
>  #include "llvm/Support/MemoryBuffer.h"
> @@ -49,6 +50,11 @@
>    ///
>    /// Return true if an error occurred.
>    bool parseMachineFunction(yaml::Input &In);
> +
> +private:
> +  /// Return a MIR diagnostic converted from an LLVM assembly diagnostic.
> +  SMDiagnostic diagFromLLVMAssemblyDiag(const SMDiagnostic &Error,
> +                                        SMRange SourceRange);
>  };
>  
>  } // end anonymous namespace
> @@ -81,8 +87,10 @@
>            dyn_cast_or_null<yaml::BlockScalarNode>(In.getCurrentNode())) {
>      M = parseAssembly(MemoryBufferRef(BSN->getValue(), Filename), Error,
>                        Context);
> -    if (!M)
> +    if (!M) {
> +      Error = diagFromLLVMAssemblyDiag(Error, BSN->getSourceRange());
>        return M;
> +    }
>      In.nextDocument();
>      if (!In.setCurrentDocument())
>        return M;
> @@ -111,6 +119,36 @@
>    return false;
>  }
>  
> +SMDiagnostic MIRParserImpl::diagFromLLVMAssemblyDiag(const SMDiagnostic &Error,
> +                                                     SMRange SourceRange) {
> +  assert(SourceRange.isValid());
> +
> +  // Translate the location of the error from the location in the llvm IR string
> +  // to the corresponding location in the MIR file.
> +  auto LineAndColumn = SM.getLineAndColumn(SourceRange.Start);
> +  unsigned Line = LineAndColumn.first + Error.getLineNo() - 1;
> +  unsigned Column = Error.getColumnNo();
> +  StringRef LineStr = Error.getLineContents();
> +  SMLoc Loc = Error.getLoc();
> +
> +  // Get the full line and adjust the column number by taking the indentation of
> +  // LLVM IR into account.
> +  line_iterator Lines(*SM.getMemoryBuffer(SM.getMainFileID()), false);
> +  while (!Lines.is_at_end() && Lines.line_number() != Line)
> +    ++Lines;
> +  if (Lines.line_number() == Line) {
> +    LineStr = *Lines;
> +    Loc = SMLoc::getFromPointer(LineStr.data());
> +    auto Indent = LineStr.find(Error.getLineContents());
> +    if (Indent != StringRef::npos)
> +      Column += Indent;
> +  }

According to the `line_iterator` docs, calling `line_number()` isn't
safe at EOF.  Your `if` statement looks like it could violate that.

I think you should change this to use a standard iterator pattern, and
only use the iterator when it's valid:

    for (line_iterator L(*SM.getMemoryBuffer(SM.getMainFileID()), false), E;
         L != E; ++L)
      if (L.line_number() == Line) {
        LineStr = *L;
        Loc = SMLoc::getFromPointer(LineStr.data());
        auto Indent = LineStr.find(Error.getLineContents());
        if (Indent != StringRef::npos)
          Column += Indent;
        break;
      }

With that change, this LGTM.

> +
> +  return SMDiagnostic(SM, Loc, Filename, Line, Column, Error.getKind(),
> +                      Error.getMessage(), LineStr, Error.getRanges(),
> +                      Error.getFixIts());
> +}
> +
>  std::unique_ptr<Module> llvm::parseMIRFile(StringRef Filename,
>                                             SMDiagnostic &Error,
>                                             LLVMContext &Context) {
> Index: test/CodeGen/MIR/llvm-ir-error-reported.mir
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/MIR/llvm-ir-error-reported.mir
> @@ -0,0 +1,22 @@
> +# RUN: not llc -start-after branch-folder -stop-after branch-folder -o /dev/null %s 2>&1 | FileCheck %s
> +# This test ensures an error is reported if the embedded LLVM IR contains an
> +# error.
> +
> +--- |
> +  
> +  ; CHECK: [[@LINE+3]]:15: error: use of undefined value '%a'
> +  define i32 @foo(i32 %x, i32 %y) {
> +    %z = alloca i32, align 4
> +    store i32 %a, i32* %z, align 4
> +    br label %Test
> +  Test:
> +    %m = load i32, i32* %z, align 4
> +    %cond = icmp eq i32 %y, %m
> +    br i1 %cond, label %IfEqual, label %IfUnequal
> +  IfEqual:
> +    ret i32 1
> +  IfUnequal:
> +    ret i32 0
> +  }
> +  
> +...
>