<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Jan 15, 2014, at 10:37 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Ping, with a rebased patch.<div><br><div apple-content-edited="true">
<div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div><div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"></div></div></div></div><span><lto_diag.patch></span></blockquote><div><br></div>See comments below.</div><div><br><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div apple-content-edited="true"><div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"></div>
</div>
<br><div><div>On Jan 9, 2014, at 11:09 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Ping.<div><br><div apple-content-edited="true">
<div style="font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div>
</div>
<br><div style=""><div>On Jan 3, 2014, at 11:38 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi,<div><br></div><div>This patch adds a way to register a diagnostic handler for the diagnostics coming from the code generator for the clients of LTO.</div><div><br></div><div>** Context **</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>** Proposed Solution **</div><div><br></div><div>Add a hook in the C API of LTO so that clients of the code generator can set their own handler.</div><div>The handler is defined like this:</div><div><font face="Menlo" style="font-size: 11px;">typedef void (*lto_diagnostic_handler_t)(lto_codegen_diagnostic_severity_t severity, const char *diag, void *ctxt)</font></div><div>- severity says how bad this is.</div><div>- diag is a string that contains the diagnostic message.</div><div>- ctxt is the registered context for this handler.</div><div><br></div><div>This hook is more general than the <font face="Menlo" style="font-size: 11px;">lot_get_error_message</font>, since this function keeps only the latest message and can only be queried when something went wrong (no warning for instance).</div><div><br></div><div>Finally, the patch also adds <font face="Menlo" style="font-size: 11px;">LTOCodeGenerator::resetDiagnosticHandler</font> (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.</div><div><br></div><div>Thanks for your reviews.</div></div></blockquote></div></div></div></blockquote></div></div></blockquote><br></div><div><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">Index: tools/lto/lto.cpp</div><div style="margin: 0px; font-family: Menlo;">===================================================================</div><div style="margin: 0px; font-family: Menlo;">--- tools/lto/lto.cpp (revision 199322)</div><div style="margin: 0px; font-family: Menlo;">+++ tools/lto/lto.cpp (working copy)</div><div style="margin: 0px; font-family: Menlo;">@@ -193,6 +193,28 @@</div><div style="margin: 0px; font-family: Menlo;"> return mod->getSymbolAttributes(index);</div><div style="margin: 0px; font-family: Menlo;"> }</div><div style="margin: 0px; font-family: Menlo; min-height: 14px;"><br></div><div style="margin: 0px; font-family: Menlo;">+/// Set a diagnostic handler.</div><div style="margin: 0px; font-family: Menlo;">+void lto_codegen_set_diagnostic_handler(lto_code_gen_t CodeGen,</div><div style="margin: 0px; font-family: Menlo;">+ lto_diagnostic_handler_t DiagHandler,</div><div style="margin: 0px; font-family: Menlo;">+ void *Ctxt) {</div></blockquote><div><br></div>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.</div><div><br></div><div><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">+ CodeGen->setDiagnosticHandler(DiagHandler, Ctxt);</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+void lto_codegen_reset_diagnostic_handler(lto_code_gen_t CodeGen) {</div><div style="margin: 0px; font-family: Menlo;">+ CodeGen->resetDiagnosticHandler();</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+/// Get the currently set diagnostic handler.</div><div style="margin: 0px; font-family: Menlo;">+lto_diagnostic_handler_t</div><div style="margin: 0px; font-family: Menlo;">+lto_codegen_get_diagnostic_handler(lto_code_gen_t CodeGen) {</div><div style="margin: 0px; font-family: Menlo;">+ return CodeGen->getDiagnosticHandler();</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+/// Get the currently get diagnostic context.</div><div style="margin: 0px; font-family: Menlo;">+void *lto_codegen_get_diagnostic_context(lto_code_gen_t CodeGen) {</div><div style="margin: 0px; font-family: Menlo;">+ return CodeGen->getDiagnosticContext();</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div></blockquote><div><br></div>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.</div><div><br><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;"> /// lto_codegen_create - Instantiates a code generator. Returns NULL if there</div><div style="margin: 0px; font-family: Menlo;"> /// is an error.</div><div style="margin: 0px; font-family: Menlo;"> lto_code_gen_t lto_codegen_create(void) {</div></blockquote><div><div style="margin: 0px;"><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;">Index: tools/lto/lto.exports</div><div style="margin: 0px;">===================================================================</div><div style="margin: 0px;">--- tools/lto/lto.exports (revision 199322)</div><div style="margin: 0px;">+++ tools/lto/lto.exports (working copy)</div><div style="margin: 0px;">@@ -15,6 +15,10 @@</div><div style="margin: 0px;"> lto_module_is_object_file_in_memory</div><div style="margin: 0px;"> lto_module_is_object_file_in_memory_for_target</div><div style="margin: 0px;"> lto_module_dispose</div><div style="margin: 0px;">+lto_codegen_set_diagnostic_handler</div><div style="margin: 0px;">+lto_codegen_get_diagnostic_handler</div><div style="margin: 0px;">+lto_codegen_get_diagnostic_context</div><div style="margin: 0px;">+lto_codegen_reset_diagnostic_handler</div></blockquote><div style="margin: 0px;"><br></div>Ditto.</div><div style="margin: 0px; font-family: Menlo;"><br></div><div style="margin: 0px;"><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;"> lto_codegen_add_module</div><div style="margin: 0px;"> lto_codegen_add_must_preserve_symbol</div><div style="margin: 0px;"> lto_codegen_compile</div></blockquote><div><div style="margin: 0px;"><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;">Index: lib/LTO/LTOCodeGenerator.cpp</div><div style="margin: 0px;">===================================================================</div><div style="margin: 0px;">--- lib/LTO/LTOCodeGenerator.cpp (revision 199322)</div><div style="margin: 0px;">+++ lib/LTO/LTOCodeGenerator.cpp (working copy)</div><div style="margin: 0px;">@@ -21,6 +21,8 @@</div><div style="margin: 0px;"> #include "llvm/IR/Constants.h"</div><div style="margin: 0px;"> #include "llvm/IR/DataLayout.h"</div><div style="margin: 0px;"> #include "llvm/IR/DerivedTypes.h"</div><div style="margin: 0px;">+#include "llvm/IR/DiagnosticInfo.h"</div><div style="margin: 0px;">+#include "llvm/IR/DiagnosticPrinter.h"</div><div style="margin: 0px;"> #include "llvm/IR/LLVMContext.h"</div><div style="margin: 0px;"> #include "llvm/IR/Mangler.h"</div><div style="margin: 0px;"> #include "llvm/IR/Module.h"</div><div style="margin: 0px;">@@ -37,6 +39,7 @@</div><div style="margin: 0px;"> #include "llvm/Support/FormattedStream.h"</div><div style="margin: 0px;"> #include "llvm/Support/Host.h"</div><div style="margin: 0px;"> #include "llvm/Support/MemoryBuffer.h"</div><div style="margin: 0px;">+#include "llvm/Support/raw_ostream.h"</div><div style="margin: 0px;"> #include "llvm/Support/Signals.h"</div><div style="margin: 0px;"> #include "llvm/Support/TargetRegistry.h"</div><div style="margin: 0px;"> #include "llvm/Support/TargetSelect.h"</div><div style="margin: 0px;">@@ -63,7 +66,9 @@</div><div style="margin: 0px;"> : Context(getGlobalContext()), Linker(new Module("ld-temp.o", Context)),</div><div style="margin: 0px;"> TargetMach(NULL), EmitDwarfDebugInfo(false), ScopeRestrictionsDone(false),</div><div style="margin: 0px;"> CodeModel(LTO_CODEGEN_PIC_MODEL_DYNAMIC),</div><div style="margin: 0px;">- InternalizeStrategy(LTO_INTERNALIZE_FULL), NativeObjectFile(NULL) {</div><div style="margin: 0px;">+ InternalizeStrategy(LTO_INTERNALIZE_FULL), NativeObjectFile(NULL),</div><div style="margin: 0px;">+ DiagHandler(NULL), DiagContext(NULL), OrigDiagHandler(NULL),</div><div style="margin: 0px;">+ OrigDiagContext(NULL) {</div></blockquote><div style="font-family: Menlo; margin: 0px;"><br></div>I think you can remove the OrigDiagHandler and OrigDiagContext if you don’t have to support the “reset_diagnostic_handler” interface, right?</div><div style="margin: 0px;"><br><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;"> initializeLTOPasses();</div><div style="margin: 0px;"> }</div><div style="margin: 0px; min-height: 14px;"><br></div></blockquote><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">@@ -536,3 +541,59 @@</div><div style="margin: 0px; font-family: Menlo;"> cl::ParseCommandLineOptions(CodegenOptions.size(),</div><div style="margin: 0px; font-family: Menlo;"> const_cast<char **>(&CodegenOptions[0]));</div><div style="margin: 0px; font-family: Menlo;"> }</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+void LTOCodeGenerator::DiagnosticHandler(const DiagnosticInfo &DI,</div><div style="margin: 0px; font-family: Menlo;">+ void *Context) {</div><div style="margin: 0px; font-family: Menlo;">+ ((LTOCodeGenerator *)Context)->DiagnosticHandler2(DI);</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+void LTOCodeGenerator::DiagnosticHandler2(const DiagnosticInfo &DI) {</div><div style="margin: 0px; font-family: Menlo;">+ // Map the LLVM internal diagnostic severity to the LTO diagnostic severity.</div><div style="margin: 0px; font-family: Menlo;">+ lto_codegen_diagnostic_severity_t Severity;</div><div style="margin: 0px; font-family: Menlo;">+ switch (DI.getSeverity()) {</div><div style="margin: 0px; font-family: Menlo;">+ case DS_Error:</div><div style="margin: 0px; font-family: Menlo;">+ Severity = LTO_DS_ERROR;</div><div style="margin: 0px; font-family: Menlo;">+ break;</div><div style="margin: 0px; font-family: Menlo;">+ case DS_Warning:</div><div style="margin: 0px; font-family: Menlo;">+ Severity = LTO_DS_WARNING;</div><div style="margin: 0px; font-family: Menlo;">+ break;</div><div style="margin: 0px; font-family: Menlo;">+ case DS_Note:</div><div style="margin: 0px; font-family: Menlo;">+ Severity = LTO_DS_NOTE;</div><div style="margin: 0px; font-family: Menlo;">+ break;</div><div style="margin: 0px; font-family: Menlo;">+ }</div><div style="margin: 0px; font-family: Menlo;">+ // Create the string that will be reported to the external diagnostic handler.</div><div style="margin: 0px; font-family: Menlo;">+ std::string MsgStorage;</div><div style="margin: 0px; font-family: Menlo;">+ raw_string_ostream Stream(MsgStorage);</div><div style="margin: 0px; font-family: Menlo;">+ DiagnosticPrinterRawOStream DP(Stream);</div><div style="margin: 0px; font-family: Menlo;">+ DI.print(DP);</div><div style="margin: 0px; font-family: Menlo;">+ Stream.flush();</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+ // If this method has been called it means someone has set up an external</div><div style="margin: 0px; font-family: Menlo;">+ // diagnostic handler. Assert on that.</div><div style="margin: 0px; font-family: Menlo;">+ assert(getDiagnosticHandler() && "Invalid diagnostic handler");</div><div style="margin: 0px; font-family: Menlo;">+ (*getDiagnosticHandler())(Severity, MsgStorage.c_str(),</div><div style="margin: 0px; font-family: Menlo;">+ getDiagnosticContext());</div><div style="margin: 0px; font-family: Menlo;">+}</div><div style="margin: 0px; font-family: Menlo;">+</div></blockquote><div><div style="margin: 0px;"><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;">+void</div><div style="margin: 0px;">+LTOCodeGenerator::setDiagnosticHandler(lto_diagnostic_handler_t DiagHandler,</div><div style="margin: 0px;">+ void *Ctxt) {</div><div style="margin: 0px;">+ this->DiagHandler = DiagHandler;</div><div style="margin: 0px;">+ this->DiagContext = Ctxt;</div><div style="margin: 0px;">+ if (!OrigDiagHandler) {</div><div style="margin: 0px;">+ OrigDiagHandler = Context.getDiagnosticHandler();</div><div style="margin: 0px;">+ OrigDiagContext = Context.getDiagnosticContext();</div><div style="margin: 0px;">+ }</div></blockquote><div style="font-family: Menlo; margin: 0px;"><br></div>Ditto.</div><div style="margin: 0px;"><br><blockquote type="cite" style="font-family: Menlo;"><div style="margin: 0px;">+ if (!DiagHandler)</div><div style="margin: 0px;">+ return Context.setDiagnosticHandler(NULL, NULL);</div><div style="margin: 0px;">+ // Register the LTOCodeGenerator stub in the LLVMContext to forward the</div><div style="margin: 0px;">+ // diagnostic to the external DiagHandler.</div><div style="margin: 0px;">+ Context.setDiagnosticHandler(LTOCodeGenerator::DiagnosticHandler, this);</div><div style="margin: 0px;">+}</div><div style="margin: 0px;">+</div><div style="margin: 0px;">+void LTOCodeGenerator::resetDiagnosticHandler() {</div><div style="margin: 0px;">+ if (OrigDiagHandler)</div><div style="margin: 0px;">+ Context.setDiagnosticHandler(OrigDiagHandler, OrigDiagContext);</div><div style="margin: 0px;">+ DiagHandler = NULL;</div><div style="margin: 0px;">+ DiagContext = NULL;</div><div style="margin: 0px;">+}</div></blockquote><div><div style="font-family: Menlo; margin: 0px;"><br></div><div style="margin: 0px;">…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.</div></div><div style="margin: 0px;"><br></div><div style="margin: 0px;">Since those are all pretty cosmetic changes, please go ahead and commit with those changes.</div></div></div></div></div></div></div></div></body></html>