[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