[PATCH][LTO] Add a hook to map LLVM diagnostics into the clients of LTO.
Quentin Colombet
qcolombet at apple.com
Wed Jan 15 14:09:58 PST 2014
Thanks Bob.
Committed revision 199338.
-Quentin
On Jan 15, 2014, at 12:51 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> On Jan 15, 2014, at 10:37 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>> Ping, with a rebased patch.
>>
>> -Quentin
>> <lto_diag.patch>
>
> See comments below.
>
>>
>> On Jan 9, 2014, at 11:09 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>
>>> Ping.
>>>
>>> -Quentin
>>>
>>> On Jan 3, 2014, at 11:38 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> This patch adds a way to register a diagnostic handler for the diagnostics coming from the code generator for the clients of LTO.
>>>>
>>>> ** Context **
>>>>
>>>> In r197438, we introduced a new API to diagnose all sorts of events in the backend. This diagnostic framework exposes a hook to map the reported events in the client of the backend.
>>>>
>>>> LTO, as a client of the backend, has access to this framework. However, LTO is not the component that directly interacts with the user. Therefore, we need to propagate these diagnostics to the clients of LTO.
>>>>
>>>>
>>>> ** Proposed Solution **
>>>>
>>>> Add a hook in the C API of LTO so that clients of the code generator can set their own handler.
>>>> The handler is defined like this:
>>>> typedef void (*lto_diagnostic_handler_t)(lto_codegen_diagnostic_severity_t severity, const char *diag, void *ctxt)
>>>> - severity says how bad this is.
>>>> - diag is a string that contains the diagnostic message.
>>>> - ctxt is the registered context for this handler.
>>>>
>>>> This hook is more general than the lot_get_error_message, since this function keeps only the latest message and can only be queried when something went wrong (no warning for instance).
>>>>
>>>> Finally, the patch also adds LTOCodeGenerator::resetDiagnosticHandler (and the related C API), to reset the diagnostic handler to the original one. Indeed, the original diagnostic handler (if any) uses the C++ API and there are no (clean) way to expose it in the C API.
>>>>
>>>> Thanks for your reviews.
>
>> Index: tools/lto/lto.cpp
>> ===================================================================
>> --- tools/lto/lto.cpp (revision 199322)
>> +++ tools/lto/lto.cpp (working copy)
>> @@ -193,6 +193,28 @@
>> return mod->getSymbolAttributes(index);
>> }
>>
>> +/// Set a diagnostic handler.
>> +void lto_codegen_set_diagnostic_handler(lto_code_gen_t CodeGen,
>> + lto_diagnostic_handler_t DiagHandler,
>> + void *Ctxt) {
>
> This is following the coding style guidelines but it’s inconsistent with the rest of the file, where (for example) the lto_code_gen_t arguments are always called “cg”. I’d prefer to keep the consistency, and then if you or someone else wants to adopt the new style, it should be done consistently for all the LTO interface code.
>
>> + CodeGen->setDiagnosticHandler(DiagHandler, Ctxt);
>> +}
>> +
>> +void lto_codegen_reset_diagnostic_handler(lto_code_gen_t CodeGen) {
>> + CodeGen->resetDiagnosticHandler();
>> +}
>> +
>> +/// Get the currently set diagnostic handler.
>> +lto_diagnostic_handler_t
>> +lto_codegen_get_diagnostic_handler(lto_code_gen_t CodeGen) {
>> + return CodeGen->getDiagnosticHandler();
>> +}
>> +
>> +/// Get the currently get diagnostic context.
>> +void *lto_codegen_get_diagnostic_context(lto_code_gen_t CodeGen) {
>> + return CodeGen->getDiagnosticContext();
>> +}
>> +
>
> I can imagine various uses for the other functions, but I don’t think there is any immediate need for them. Let’s just stick with lto_codegen_set_diagnostic_handler for now. We can always add things later, but it’s hard to ever remove anything from the LTO interface.
>
>> /// lto_codegen_create - Instantiates a code generator. Returns NULL if there
>> /// is an error.
>> lto_code_gen_t lto_codegen_create(void) {
>> Index: tools/lto/lto.exports
>> ===================================================================
>> --- tools/lto/lto.exports (revision 199322)
>> +++ tools/lto/lto.exports (working copy)
>> @@ -15,6 +15,10 @@
>> lto_module_is_object_file_in_memory
>> lto_module_is_object_file_in_memory_for_target
>> lto_module_dispose
>> +lto_codegen_set_diagnostic_handler
>> +lto_codegen_get_diagnostic_handler
>> +lto_codegen_get_diagnostic_context
>> +lto_codegen_reset_diagnostic_handler
>
> Ditto.
>
>> lto_codegen_add_module
>> lto_codegen_add_must_preserve_symbol
>> lto_codegen_compile
>> Index: lib/LTO/LTOCodeGenerator.cpp
>> ===================================================================
>> --- lib/LTO/LTOCodeGenerator.cpp (revision 199322)
>> +++ lib/LTO/LTOCodeGenerator.cpp (working copy)
>> @@ -21,6 +21,8 @@
>> #include "llvm/IR/Constants.h"
>> #include "llvm/IR/DataLayout.h"
>> #include "llvm/IR/DerivedTypes.h"
>> +#include "llvm/IR/DiagnosticInfo.h"
>> +#include "llvm/IR/DiagnosticPrinter.h"
>> #include "llvm/IR/LLVMContext.h"
>> #include "llvm/IR/Mangler.h"
>> #include "llvm/IR/Module.h"
>> @@ -37,6 +39,7 @@
>> #include "llvm/Support/FormattedStream.h"
>> #include "llvm/Support/Host.h"
>> #include "llvm/Support/MemoryBuffer.h"
>> +#include "llvm/Support/raw_ostream.h"
>> #include "llvm/Support/Signals.h"
>> #include "llvm/Support/TargetRegistry.h"
>> #include "llvm/Support/TargetSelect.h"
>> @@ -63,7 +66,9 @@
>> : Context(getGlobalContext()), Linker(new Module("ld-temp.o", Context)),
>> TargetMach(NULL), EmitDwarfDebugInfo(false), ScopeRestrictionsDone(false),
>> CodeModel(LTO_CODEGEN_PIC_MODEL_DYNAMIC),
>> - InternalizeStrategy(LTO_INTERNALIZE_FULL), NativeObjectFile(NULL) {
>> + InternalizeStrategy(LTO_INTERNALIZE_FULL), NativeObjectFile(NULL),
>> + DiagHandler(NULL), DiagContext(NULL), OrigDiagHandler(NULL),
>> + OrigDiagContext(NULL) {
>
> I think you can remove the OrigDiagHandler and OrigDiagContext if you don’t have to support the “reset_diagnostic_handler” interface, right?
>
>> initializeLTOPasses();
>> }
>>
>> @@ -536,3 +541,59 @@
>> cl::ParseCommandLineOptions(CodegenOptions.size(),
>> const_cast<char **>(&CodegenOptions[0]));
>> }
>> +
>> +void LTOCodeGenerator::DiagnosticHandler(const DiagnosticInfo &DI,
>> + void *Context) {
>> + ((LTOCodeGenerator *)Context)->DiagnosticHandler2(DI);
>> +}
>> +
>> +void LTOCodeGenerator::DiagnosticHandler2(const DiagnosticInfo &DI) {
>> + // Map the LLVM internal diagnostic severity to the LTO diagnostic severity.
>> + lto_codegen_diagnostic_severity_t Severity;
>> + switch (DI.getSeverity()) {
>> + case DS_Error:
>> + Severity = LTO_DS_ERROR;
>> + break;
>> + case DS_Warning:
>> + Severity = LTO_DS_WARNING;
>> + break;
>> + case DS_Note:
>> + Severity = LTO_DS_NOTE;
>> + break;
>> + }
>> + // Create the string that will be reported to the external diagnostic handler.
>> + std::string MsgStorage;
>> + raw_string_ostream Stream(MsgStorage);
>> + DiagnosticPrinterRawOStream DP(Stream);
>> + DI.print(DP);
>> + Stream.flush();
>> +
>> + // If this method has been called it means someone has set up an external
>> + // diagnostic handler. Assert on that.
>> + assert(getDiagnosticHandler() && "Invalid diagnostic handler");
>> + (*getDiagnosticHandler())(Severity, MsgStorage.c_str(),
>> + getDiagnosticContext());
>> +}
>> +
>> +void
>> +LTOCodeGenerator::setDiagnosticHandler(lto_diagnostic_handler_t DiagHandler,
>> + void *Ctxt) {
>> + this->DiagHandler = DiagHandler;
>> + this->DiagContext = Ctxt;
>> + if (!OrigDiagHandler) {
>> + OrigDiagHandler = Context.getDiagnosticHandler();
>> + OrigDiagContext = Context.getDiagnosticContext();
>> + }
>
> Ditto.
>
>> + if (!DiagHandler)
>> + return Context.setDiagnosticHandler(NULL, NULL);
>> + // Register the LTOCodeGenerator stub in the LLVMContext to forward the
>> + // diagnostic to the external DiagHandler.
>> + Context.setDiagnosticHandler(LTOCodeGenerator::DiagnosticHandler, this);
>> +}
>> +
>> +void LTOCodeGenerator::resetDiagnosticHandler() {
>> + if (OrigDiagHandler)
>> + Context.setDiagnosticHandler(OrigDiagHandler, OrigDiagContext);
>> + DiagHandler = NULL;
>> + DiagContext = NULL;
>> +}
>
> …and again here. And the same comments in apply in various other places later in the patch, but they’re obvious and I’m not going to call them all out.
>
> Since those are all pretty cosmetic changes, please go ahead and commit with those changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140115/dbcf59e6/attachment.html>
More information about the llvm-commits
mailing list