[llvm] r225562 - Use the DiagnosticHandler to print diagnostics when reading bitcode.

Duncan P. N. Exon Smith dexonsmith at apple.com
Tue Feb 24 11:59:03 PST 2015


> On 2015-Jan-09, at 16:07, Rafael Espindola <rafael.espindola at gmail.com> wrote:
> 
> Author: rafael
> Date: Fri Jan  9 18:07:30 2015
> New Revision: 225562
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=225562&view=rev
> Log:
> Use the DiagnosticHandler to print diagnostics when reading bitcode.
> 
> The bitcode reading interface used std::error_code to report an error to the
> callers and it is the callers job to print diagnostics.
> 
> This is not ideal for error handling or diagnostic reporting:
> 
> * For error handling, all that the callers care about is 3 possibilities:
>  * It worked
>  * The bitcode file is corrupted/invalid.
>  * The file is not bitcode at all.
> 
> * For diagnostic, it is user friendly to include far more information
>  about the invalid case so the user can find out what is wrong with the
>  bitcode file. This comes up, for example, when a developer introduces a
>  bug while extending the format.
> 
> The compromise we had was to have a lot of error codes.
> 
> With this patch we use the DiagnosticHandler to communicate with the
> human and std::error_code to communicate with the caller.
> 
> This allows us to have far fewer error codes and adds the infrastructure to
> print better diagnostics. This is so because the diagnostics are printed when
> he issue is found. The code that detected the problem in alive in the stack and
> can pass down as much context as needed. As an example the patch updates
> test/Bitcode/invalid.ll.
> 
> Using a DiagnosticHandler also moves the fatal/non-fatal error decision to the
> caller. A simple one like llvm-dis can just use fatal errors. The gold plugin
> needs a bit more complex treatment because of being passed non-bitcode files. An
> hypothetical interactive tool would make all bitcode errors non-fatal.
> 
> Modified:
>    llvm/trunk/include/llvm/Bitcode/ReaderWriter.h
>    llvm/trunk/include/llvm/IR/DiagnosticInfo.h
>    llvm/trunk/include/llvm/Linker/Linker.h
>    llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>    llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h
>    llvm/trunk/lib/LTO/LTOModule.cpp
>    llvm/trunk/lib/Linker/LinkModules.cpp
>    llvm/trunk/test/Bitcode/invalid.ll
>    llvm/trunk/test/LTO/invalid.ll
>    llvm/trunk/tools/gold/gold-plugin.cpp
>    llvm/trunk/tools/llvm-dis/llvm-dis.cpp

Turns out we've had warnings when reading bitcode since r199346, which...

> Modified: llvm/trunk/tools/gold/gold-plugin.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/gold/gold-plugin.cpp?rev=225562&r1=225561&r2=225562&view=diff
> ==============================================================================
> --- llvm/trunk/tools/gold/gold-plugin.cpp (original)
> +++ llvm/trunk/tools/gold/gold-plugin.cpp Fri Jan  9 18:07:30 2015
> @@ -19,6 +19,8 @@
> #include "llvm/CodeGen/Analysis.h"
> #include "llvm/CodeGen/CommandFlags.h"
> #include "llvm/IR/Constants.h"
> +#include "llvm/IR/DiagnosticInfo.h"
> +#include "llvm/IR/DiagnosticPrinter.h"
> #include "llvm/IR/LLVMContext.h"
> #include "llvm/IR/Module.h"
> #include "llvm/IR/Verifier.h"
> @@ -269,6 +271,23 @@ static bool shouldSkip(uint32_t Symflags
>   return false;
> }
> 
> +static void diagnosticHandler(const DiagnosticInfo &DI, void *Context) {
> +  assert(DI.getSeverity() == DS_Error && "Only expecting errors");

... causes a crash here and...

> +  const auto &BDI = cast<BitcodeDiagnosticInfo>(DI);
> +  std::error_code EC = BDI.getError();
> +  if (EC == BitcodeError::InvalidBitcodeSignature)
> +    return;
> +
> +  std::string ErrStorage;
> +  {
> +    raw_string_ostream OS(ErrStorage);
> +    DiagnosticPrinterRawOStream DP(OS);
> +    DI.print(DP);
> +  }
> +  message(LDPL_FATAL, "LLVM gold plugin has failed to create LTO module: %s",
> +          ErrStorage.c_str());
> +}
> +
> 
> Modified: llvm/trunk/tools/llvm-dis/llvm-dis.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-dis/llvm-dis.cpp?rev=225562&r1=225561&r2=225562&view=diff
> ==============================================================================
> --- llvm/trunk/tools/llvm-dis/llvm-dis.cpp (original)
> +++ llvm/trunk/tools/llvm-dis/llvm-dis.cpp Fri Jan  9 18:07:30 2015
> @@ -20,6 +20,8 @@
> #include "llvm/Bitcode/ReaderWriter.h"
> #include "llvm/IR/AssemblyAnnotationWriter.h"
> #include "llvm/IR/DebugInfo.h"
> +#include "llvm/IR/DiagnosticInfo.h"
> +#include "llvm/IR/DiagnosticPrinter.h"
> #include "llvm/IR/IntrinsicInst.h"
> #include "llvm/IR/Module.h"
> #include "llvm/IR/Type.h"
> @@ -112,6 +114,17 @@ public:
> 
> } // end anon namespace
> 
> +static void diagnosticHandler(const DiagnosticInfo &DI, void *Context) {
> +  assert(DI.getSeverity() == DS_Error && "Only expecting errors");

... here.

(The testcase at test/Bitcode/drop-debug-info.ll didn't check
reading from bitcode -- just from assembly -- so it didn't fire when
you made this change.)

I was thinking of removing the `assert()` and...

> +
> +  raw_ostream &OS = errs();
> +  OS << (char *)Context << ": ";

... and printing "error"/"warning"/etc. here and...

> +  DiagnosticPrinterRawOStream DP(OS);
> +  DI.print(DP);
> +  OS << '\n';
> +  exit(1);

... and changing this to:

    if (DI.getSeverity() == DS_Error)
      exit(1);

I figure you'd need a similar change to gold-plugin, but I'd have
to leave that up to someone that knows the gold API.

Thoughts?

> +}
> +

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Bitcode-Stop-crashing-when-dropping-debug-info.patch
Type: application/octet-stream
Size: 11125 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150224/382b7046/attachment.obj>


More information about the llvm-commits mailing list