[PATCH] Make inline assembly parse failures a warning unless the integrated assembler is required for the TargetStreamer.

[Rafael Espíndola]

There is currently a thread on llvmdev about what we should do about
this. I would suggest putting this review on hold for the moment and
move the discussion there. If we decide to warn, we can move it back
here.

On 21 February 2014 11:30, Daniel Sanders <daniel.sanders at imgtec.com> wrote:
> Hi rafael, rengolin, dwmw2, compnerd,
>
> When the TargetStreamer requires the integrated assembler (i.e. it is an object
> format), continue to emit errors on parse failures as we currently do.
> Otherwise, report warnings instead.
>
> There is a clang counterpart to this patch which should only consist of a test
> case for 'clang -[ESc] {,-no-integrated-as}'. At the moment, I'm finding the
> -no-integrated-as option doesn't correctly disable warnings for the
> 'clang -S -no-integrated-as' case.
>
> http://llvm-reviews.chandlerc.com/D2859
>
> Files:
>   include/llvm/Support/SourceMgr.h
>   lib/Support/SourceMgr.cpp
>   lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
>   test/CodeGen/Generic/invalid-inline-asm.ll
>
> Index: include/llvm/Support/SourceMgr.h
> ===================================================================
> --- include/llvm/Support/SourceMgr.h
> +++ include/llvm/Support/SourceMgr.h
> @@ -44,6 +44,11 @@
>    /// custom way can register a function pointer+context as a diagnostic
>    /// handler.  It gets called each time PrintMessage is invoked.
>    typedef void (*DiagHandlerTy)(const SMDiagnostic &, void *Context);
> +
> +  /// DiagMutatorTy - Clients that want to modify diagnostics in a custom way
> +  /// can register a function pointer as a diagnostic mutator. It gets called
> +  /// each time GetMessage is invoked.
> +  typedef void (*DiagMutatorTy)(SMDiagnostic &);
>  private:
>    struct SrcBuffer {
>      /// Buffer - The memory buffer for the file.
> @@ -66,12 +71,14 @@
>    mutable void *LineNoCache;
>
>    DiagHandlerTy DiagHandler;
> +  DiagMutatorTy DiagMutator;
>    void *DiagContext;
>
>    SourceMgr(const SourceMgr&) LLVM_DELETED_FUNCTION;
>    void operator=(const SourceMgr&) LLVM_DELETED_FUNCTION;
>  public:
> -  SourceMgr() : LineNoCache(0), DiagHandler(0), DiagContext(0) {}
> +  SourceMgr() : LineNoCache(0), DiagHandler(0), DiagMutator(0), DiagContext(0) {
> +  }
>    ~SourceMgr();
>
>    void setIncludeDirs(const std::vector<std::string> &Dirs) {
> @@ -86,6 +93,13 @@
>    }
>
>    DiagHandlerTy getDiagHandler() const { return DiagHandler; }
> +
> +  /// Specify a diagnostic mutator to be invoked every time GetMessage is
> +  /// called.
> +  void setDiagMutator(DiagMutatorTy DH) { DiagMutator = DH; }
> +
> +  DiagMutatorTy getDiagMutator() const { return DiagMutator; }
> +
>    void *getDiagContext() const { return DiagContext; }
>
>    const SrcBuffer &getBufferInfo(unsigned i) const {
> Index: lib/Support/SourceMgr.cpp
> ===================================================================
> --- lib/Support/SourceMgr.cpp
> +++ lib/Support/SourceMgr.cpp
> @@ -206,9 +206,13 @@
>      LineAndCol = getLineAndColumn(Loc, CurBuf);
>    }
>
> -  return SMDiagnostic(*this, Loc, BufferID, LineAndCol.first,
> -                      LineAndCol.second-1, Kind, Msg.str(),
> -                      LineStr, ColRanges, FixIts);
> +  SMDiagnostic Diag(*this, Loc, BufferID, LineAndCol.first, LineAndCol.second-1,
> +                    Kind, Msg.str(), LineStr, ColRanges, FixIts);
> +
> +  if (DiagMutator)
> +    DiagMutator(Diag);
> +
> +  return Diag;
>  }
>
>  void SourceMgr::PrintMessage(raw_ostream &OS, SMLoc Loc,
> Index: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
> ===================================================================
> --- lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
> +++ lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
> @@ -45,6 +45,19 @@
>    };
>  }
>
> +static void MutateErrorsToWarnings(SMDiagnostic &Diag) {
> +  SourceMgr::DiagKind DK = Diag.getKind();
> +
> +  if (DK != SourceMgr::DK_Error)
> +    return;
> +
> +  SMDiagnostic NewDiag(*Diag.getSourceMgr(), Diag.getLoc(), Diag.getFilename(),
> +                       Diag.getLineNo(), Diag.getColumnNo(), SourceMgr::DK_Warning,
> +                       Diag.getMessage(), Diag.getLineContents(),
> +                       Diag.getRanges(), Diag.getFixIts());
> +  Diag = NewDiag;
> +}
> +
>  /// srcMgrDiagHandler - This callback is invoked when the SourceMgr for an
>  /// inline asm has an error in it.  diagInfo is a pointer to the SrcMgrDiagInfo
>  /// struct above.
> @@ -109,6 +122,11 @@
>      HasDiagHandler = true;
>    }
>
> +  if (!OutStreamer.isIntegratedAssemblerRequired()) {
> +    assert(!SrcMgr.getDiagMutator() && "Mutator already configured");
> +    SrcMgr.setDiagMutator(MutateErrorsToWarnings);
> +  }
> +
>    MemoryBuffer *Buffer;
>    if (isNullTerminated)
>      Buffer = MemoryBuffer::getMemBuffer(Str, "<inline asm>");
> @@ -147,8 +165,19 @@
>    int Res = Parser->Run(/*NoInitialTextSection*/ true,
>                          /*NoFinalize*/ true);
>    emitInlineAsmEnd(STIOrig, STI.get());
> -  if (Res && !HasDiagHandler)
> -    report_fatal_error("Error parsing inline asm\n");
> +
> +  if (Res) {
> +    // Emit an overall 'parse failed' diagnostic in the appropriate way.
> +    if (!HasDiagHandler) {
> +      if (OutStreamer.isIntegratedAssemblerRequired())
> +        OutContext.FatalError(SMLoc(), "Could not parse inline asm.");
> +      else
> +        errs() << "LLVM WARNING: Could not parse inline asm.\n";
> +    }
> +    else
> +      SrcMgr.PrintMessage(SMLoc(), SourceMgr::DK_Error,
> +                          "Could not parse inline asm.");
> +  }
>  }
>
>  static void EmitMSInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
> Index: test/CodeGen/Generic/invalid-inline-asm.ll
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/Generic/invalid-inline-asm.ll
> @@ -0,0 +1,27 @@
> +; -no-integrated-as disables all parsing. No errors, no warnings.
> +; RUN: llc -no-integrated-as < %s 2>&1 | FileCheck -check-prefix=IAS-SILENT %s
> +
> +; Parse, but only warn.
> +; RUN: llc < %s >/dev/null 2>%t1
> +; RUN: FileCheck -check-prefix=IAS-WARNING %s < %t1
> +
> +; Parse and error (-filetype=obj forces parsing).
> +; RUN: not llc -filetype=obj < %s >/dev/null 2>%t2
> +; RUN: FileCheck -check-prefix=IAS-ERROR %s < %t2
> +
> +; Parse and error (-filetype=obj overrules -no-integrated-as).
> +; RUN: not llc -no-integrated-as -filetype=obj < %s >/dev/null 2>%t3
> +; RUN: FileCheck -check-prefix=IAS-ERROR %s < %t3
> +
> +define void @foo() {
> +        call void asm sideeffect "this is almost certainly invalid", ""()
> +        ret void
> +}
> +
> +; IAS-SILENT-NOT: LLVM ERROR: Could not parse inline asm
> +
> +; IAS-WARNING: <inline asm>:{{.*}} warning:
> +; IAS-WARNING: LLVM WARNING: Could not parse inline asm
> +
> +; IAS-ERROR: <inline asm>:{{.*}} error:
> +; IAS-ERROR: LLVM ERROR: Could not parse inline asm