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

[Daniel Sanders]

Fair enough. The llvmdev thread only started ~30mins ago so I hadn't seen it at the point I posted this patch.

> -----Original Message-----
> From: Rafael Espíndola [mailto:rafael.espindola at gmail.com]
> Sent: 21 February 2014 16:35
> To: reviews+D2859+public+1831a21de63e9046 at llvm-reviews.chandlerc.com
> Cc: Renato Golin; David Woodhouse; Saleem Abdulrasool; Daniel Sanders;
> llvm-commits
> Subject: Re: [PATCH] Make inline assembly parse failures a warning unless
> the integrated assembler is required for the TargetStreamer.
> 
> 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