Add a libLTO diagnostic handler that supports lto_get_error_message API

[ygao]

Summary

Hi Rafael, Duncan, Peter,
This is a follow-up from the previous discussion on the thread:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151019/307763.html

The LibLTO lto_get_error_message() API reads error messages from a std::string sLastErrorString.
Instead of passing this string around as an argument, this patch creates a diagnostic handler and
then sends this handler to the constructor of LTOCodeGenerator.

There are still a few cases inside lto.cpp where sLastErrorString is being passed around as an
argument. I'd like to look into cleaning those up in a separate patch.

This patch did not addressing testing: llvm-lto uses a custom diagnostic handler; maybe I can
overwrite that custom handler with a new handler, but then I would be testing the new handler
in llvm-lto.cpp instead of the handler in lto.cpp. I'll have to think about it.

• Gao

[mehdi_amini]

include/llvm/LTO/LTOCodeGenerator.h

68 ↗

(On Diff #39140)

The combinatory explosion of constructors is not nice. I feel we should be able to avoid it with a default value (and promoting handleLTODiagnostic to a static class method):

struct LTOCodeGenerator {
  static void handleLTODiagnostic(const DiagnosticInfo &DI);
  LTOCodeGenerator(DiagnosticHandlerFunction DiagnosticHandler = handleLTODiagnostic);
  ....

tools/lto/lto.cpp

93

An issue I had the last few days with this is that when multiple diagnostic are issued, they will be concatenated in a single giant line, should they be split on a new line here?

[ygao]

I am actually not too satisfied with this patch myself, because I realize that the LTO client can call lto_codegen_set_diagnostic_handler() to set a new diagnostic handler, which however does not seem to change the handleLibLTODiagnostic() method installed by this patch here (the "IRLinker" member of the LTOCodeGenerator class does not provide a method to change its diagnostic handler).

include/llvm/LTO/LTOCodeGenerator.h

68 ↗

(On Diff #39140)

Thanks for looking at this. Yes that looks more clean.

[• rafael]

I am actually not too satisfied with this patch myself, because I realize that the LTO client can call lto_codegen_set_diagnostic_handler() to set a new diagnostic handler, which however does not seem to change the handleLibLTODiagnostic() method installed by this patch here (the "IRLinker" member of the LTOCodeGenerator class does not provide a method to change its diagnostic handler).

Interesting.

Since we give access to the diagnostic handler, could we just
deprecate and eventually remove lto_get_error_message?

As for changing the diagnostic handler. It seems odd to support
changing it in the middle of the link. What I would suggest is
changing IRLinker to a std::unique_ptr that is constructed on first
use and documented that lto_codegen_set_diagnostic_handler cannot be
used once setModule or addModule has been called.

Cheers,
Rafael

[ygao]

Well it is possible that the LTO client may call lto_codegen_create and then
immediately call lto_codegen_set_diagnotic_handler, before setModule or
addModule. In that case, if lto_codegen_create has installed a default
handler, it should be allowed to be updated.

I looked at the llvm::Linker's constructor again. So, the default diagnostic
handler there is LLVMContext::diagnose(). LLVMContext::diagnose() will check
first whether there has been a custom diagnostic handler installed, in which
case it will dispatch to that custom handler instead of the default behavior
of printing the diagnostic message and exiting. So my idea is to call
setDiagnosticHandler() in the constructor of LTOCodeGenerator, which sets
a custom dianostic handler in the associated LLVMContext as well. Inside
LTOCodeGenerator.cpp, I can give it a default non-exiting handler that prints
to stderr; and in lto.cpp, I can give it a handler that prints to
sLastErrorString.

[• rafael]

I think all that you need is the change to the constructor call of IRLinker and the current change in tools/* no?

lib/LTO/LTOCodeGenerator.cpp

79

The switch is already fully covered, no?

83

Since this is code in lib/, it would probably better to not set the diagonstic handler at all.

[ygao]

Also, I'd like to make some trivial spelling changes to llvm-lto.cpp to verify that setDiagnosticHandler() actually did change the handler (but it's possible that it is already verified in other forms that I am not aware of, in which case I'm happy to take out my changes).

lib/LTO/LTOCodeGenerator.cpp

83

True, I do not have to set the diagnostic handler in lib/; I only need to set it for tools/lto.cpp, so I can take out pretty much the changes in r247461. I am thinking to keep the test case to verify that in the case if setDiagnosticHandler() is called, the LTO library does not call exit(1).

[mehdi_amini]

tools/llvm-lto/llvm-lto.cpp

115 ↗

(On Diff #39739)

These changes are unrelated to the rest of this patch right? Please split if it is the case.

[ygao]

I am taking out the changes to llvm-lto.cpp for a separate patch.

[• rafael]

This is fine by me.

Pleas just wait for another LGTM form someone that uses the C api (Duncan?).

[mehdi_amini]

LGTM.

lib/LTO/LTOCodeGenerator.cpp

 #endif
 }
 
-static void handleLTODiagnostic(const DiagnosticInfo &DI) {
-  DiagnosticPrinterRawOStream DP(errs());                                
-  DI.print(DP);                                                          
-  errs() << "\n";
-}
-
 LTOCodeGenerator::LTOCodeGenerator()
     : Context(getGlobalContext()),
       MergedModule(new Module("ld-temp.o", Context)),
-      IRLinker(MergedModule.get(), handleLTODiagnostic) {
+      IRLinker(MergedModule.get()) {
   initializeLTOPasses();
 }
 

 LTOCodeGenerator::LTOCodeGenerator(std::unique_ptr<LLVMContext> Context)
     : OwnedContext(std::move(Context)), Context(*OwnedContext),
       MergedModule(new Module("ld-temp.o", *OwnedContext)),
-      IRLinker(MergedModule.get(), handleLTODiagnostic) {
+      IRLinker(MergedModule.get()) {
   initializeLTOPasses();
 }
 

test/LTO/X86/diagnostic-handler-noexit.ll

 
 ; RUN: llvm-as <%s >%t1
 ; RUN: llvm-as <%s >%t2
-; RUN: not llvm-lto -o /dev/null %t1 %t2 2>&1 | FileCheck %s
+; RUN: not llvm-lto -use-diagnostic-handler -o /dev/null %t1 %t2 2>&1 | FileCheck %s
 
 target triple = "x86_64-unknown-linux-gnu"
 
-; CHECK: Linking globals named 'goodboy': symbol multiply defined!
+; CHECK: error: Linking globals named 'goodboy': symbol multiply defined!
 ; CHECK: llvm-lto{{.*}}: error adding file
 @goodboy = global i32 3203383023, align 4    ; 0xbeefbeef

tools/lto/lto.cpp

 
 namespace {
 
+static void handleLibLTODiagnostic(lto_codegen_diagnostic_severity_t Severity,
+                                   const char *Msg, void *) {
+  sLastErrorString = Msg;
+  sLastErrorString += "\n";
+}
+
 // This derived class owns the native object file. This helps implement the
 // libLTO API semantics, which require that the code generator owns the object
 // file.
 struct LibLTOCodeGenerator : LTOCodeGenerator {
-  LibLTOCodeGenerator() {}
+  LibLTOCodeGenerator() {
+    setDiagnosticHandler(handleLibLTODiagnostic, nullptr); }
   LibLTOCodeGenerator(std::unique_ptr<LLVMContext> Context)
-      : LTOCodeGenerator(std::move(Context)) {}
+      : LTOCodeGenerator(std::move(Context)) {
+    setDiagnosticHandler(handleLibLTODiagnostic, nullptr); }
 
   std::unique_ptr<MemoryBuffer> NativeObjectFile;
 };