[ThinLTO][LTO][Legacy] Fix dependent libraries support by adding querying of the IRSymtab

[bd1976llvm]

Summary

Dependent libraries support for the legacy api was committed in a broken state (see: https://reviews.llvm.org/D60274). This was missed due to the painful nature of having to integrate the changes into a linker in order to test. This change implements support for dependent libraries in the legacy LTO api:

• I have removed the current api function, which returns a single string, and added functions to access each dependent library specifier individually.
• To reduce the testing pain, I have made the api functions as thin as possible to maximize coverage from llvm-lto.
• When doing ThinLTO the system linker will load the modules lazily when scanning the input files. Unfortunately, when modules are lazily loaded there is no access to module level named metadata. To fix this I have added api functions that allow querying the IRSymtab for the dependent libraries. I hope to expand the api in the future so that, eventually, all the information needed by a client linker during scan can be retrieved from the IRSymtab.

[steven_wu]

This is ELF specific and miss the initial RFC so there is quite some reading to catch up. Should we merge the MachO implementation together with the ELF one? Once you commit this, you might want to file a bug report on me to catch up on the macho side.

Can you also add an integrated testcase (with llvm-lto) to show how this should be integrated with rest of the pipeline? There can be some interesting corner cases, like pulling in a static library with another dependency library (which might or might not already satisfied).

From the first glance, the patch looks fine to me.

[bd1976llvm]

Thanks for the response.

In D62935#1534216, @steven_wu wrote:

This is ELF specific and miss the initial RFC so there is quite some reading to catch up. Should we merge the MachO implementation together with the ELF one? Once you commit this, you might want to file a bug report on me to catch up on the macho side.

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

Can you also add an integrated testcase (with llvm-lto) to show how this should be integrated with rest of the pipeline? There can be some interesting corner cases, like pulling in a static library with another dependency library (which might or might not already satisfied).

I might be misunderstanding here, but I don't think I can provide any further testing. These api functions are intended to be used by a client linker so that it can do symbol resolution (including processing the dependent libraries) on bitcode files. This is done before invoking the Full/ThinLTO codegen to generate native object files from the input bitcode files. So all we need to do is test that the correct information is supplied to a client linker. How to resolve scenarios such as pulling in a static library which specifies a dependent library is up to the client linker, no?

From the first glance, the patch looks fine to me.

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

[steven_wu]

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

It would be good if the API is flexible to return either. Not against having a different API to return linker options but I think that is less clean.

I might be misunderstanding here, but I don't think I can provide any further testing. These api functions are intended to be used by a client linker so that it can do symbol resolution (including processing the dependent libraries) on bitcode files. This is done before invoking the Full/ThinLTO codegen to generate native object files from the input bitcode files. So all we need to do is test that the correct information is supplied to a client linker. How to resolve scenarios such as pulling in a static library which specifies a dependent library is up to the client linker, no?

I agree it should be in the linker implementation.

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

This is fine. Do you think you might want to query symbols from InputFile instead of LTOModule in the future?

[bd1976llvm]

In D62935#1537096, @steven_wu wrote:

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

It would be good if the API is flexible to return either. Not against having a different API to return linker options but I think that is less clean.

Actually, now I remember that we need both. Developers do want to use linker options on ELF in the future (LLVM retains support for linker options on ELF - https://llvm.org/docs/Extensions.html#linker-options-section-linker-options). It is not implemented yet in LLD; but, I believe that the idea is that in source code "#pragma comment(lib, "foo") writes "foo" into the llvm.dependent-libraries and some new pragma e.g. #pragma linker(lib, "foo") would write ("lib", "foo") into llvm.linker-options which will specify -lfoo to ELF linkers. Given that, I think that we need to have separate api functions in the LTO interface.

[bd1976llvm]

In D62935#1537096, @steven_wu wrote:

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

This is fine. Do you think you might want to query symbols from InputFile instead of LTOModule in the future?

Yes! I would like to expand the api in the future so that a client linker can everything it needs from the IRSymtab.

[steven_wu]

SGTM

llvm/trunk/include/llvm-c/lto.h

  * responsibility to split the flags using a platform-specific mechanism.
  *
  * \since LTO_API_VERSION=16
  */
 extern const char*
 lto_module_get_linkeropts(lto_module_t mod);
 
 /**
-* Returns the module's dependent library specifiers.
-*
-* \since LTO_API_VERSION=24
-*/
-extern const char*
-lto_module_get_dependent_libraries(lto_module_t mod);
-
-/**
  * Diagnostic severity.
  *
  * \since LTO_API_VERSION=7
  */
 typedef enum {
   LTO_DS_ERROR = 0,
   LTO_DS_WARNING = 1,
   LTO_DS_REMARK = 3, // Added in LTO_API_VERSION=10.

  * Sets the maximum number of files in the cache directory. An unspecified
  * default value will be applied. A value of 0 will be ignored.
  *
  * \since LTO_API_VERSION=22
  */
 extern void thinlto_codegen_set_cache_size_files(thinlto_code_gen_t cg,
                                                  unsigned max_size_files);
 
+/** Opaque reference to an LTO input file */
+typedef struct LLVMOpaqueLTOInput *lto_input_t;
+
+/**
+  * Creates an LTO input file from a buffer. The path
+  * argument is used for diagnotics as this function
+  * otherwise does not know which file the given buffer
+  * is associated with.
+  *
+  * \since LTO_API_VERSION=24
+  */
+extern lto_input_t lto_input_create(const void *buffer,
+                                    size_t buffer_size,
+                                    const char *path);
 
+/**
+  * Frees all memory internally allocated by the LTO input file.
+  * Upon return the lto_module_t is no longer valid.
+  *
+  * \since LTO_API_VERSION=24
+  */
+extern void lto_input_dispose(lto_input_t input);
+
+/**
+  * Returns the number of dependent library specifiers
+  * for the given LTO input file.
+  *
+  * \since LTO_API_VERSION=24
+  */
+extern unsigned lto_input_get_num_dependent_libraries(lto_input_t input);
+
+/**
+  * Returns the ith dependent library specifier
+  * for the given LTO input file. The returned
+  * string is not null-terminated.
+  *
+  * \since LTO_API_VERSION=24
+  */
+extern const char * lto_input_get_dependent_library(lto_input_t input,
+                                                    size_t index,
+                                                    size_t *size);
 
 /**
  * @} // endgroup LLVMCTLTO_CACHING
  */
 
 #ifdef __cplusplus
 }
 #endif
 
 #endif /* LLVM_C_LTO_H */

llvm/trunk/include/llvm/LTO/legacy/LTOModule.h

 
 #ifndef LLVM_LTO_LTOMODULE_H
 #define LLVM_LTO_LTOMODULE_H
 
 #include "llvm-c/lto.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/IR/Module.h"
+#include "llvm/LTO/LTO.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Object/ModuleSymbolTable.h"
 #include "llvm/Target/TargetMachine.h"
 #include <string>
 #include <vector>
 
 // Forward references to llvm classes.
 namespace llvm {
public:
   const GlobalValue *getSymbolGV(uint32_t index) {
     if (index < _symbols.size())
       return _symbols[index].symbol;
     return nullptr;
   }
 
   StringRef getLinkerOpts() { return LinkerOpts; }
 
-  StringRef getDependentLibraries() { return DependentLibraries; }
-
   const std::vector<StringRef> &getAsmUndefinedRefs() { return _asm_undefines; }
 
+  static lto::InputFile *createInputFile(const void *buffer, size_t buffer_size,
+                                         const char *path, std::string &out_error);
+
+  static size_t getDependentLibraryCount(lto::InputFile *input);
+
+  static const char *getDependentLibrary(lto::InputFile *input, size_t index, size_t *size);
+
 private:
   /// Parse metadata from the module
   // FIXME: it only parses "llvm.linker.options" metadata at the moment
+  // FIXME: can't access metadata in lazily loaded modules
   void parseMetadata();
 
   /// Parse the symbols from the module and model-level ASM and add them to
   /// either the defined or undefined lists.
   void parseSymbols();
 
   /// Add a symbol which isn't defined just yet to a list to be resolved later.
   void addPotentialUndefinedSymbol(ModuleSymbolTable::Symbol Sym,

llvm/trunk/lib/LTO/LTOModule.cpp

void LTOModule::parseMetadata() {
   if (!TT.isOSBinFormatCOFF())
     return;
   Mangler M;
   for (const NameAndAttributes &Sym : _symbols) {
     if (!Sym.symbol)
       continue;
     emitLinkerFlagsForGlobalCOFF(OS, Sym.symbol, TT, M);
   }
+}
+
+lto::InputFile *LTOModule::createInputFile(const void *buffer,
+                                           size_t buffer_size, const char *path,
+                                           std::string &outErr) {
+  StringRef Data((const char *)buffer, buffer_size);
+  MemoryBufferRef BufferRef(Data, path);
+
+  Expected<std::unique_ptr<lto::InputFile>> ObjOrErr =
+      lto::InputFile::create(BufferRef);
+
+  if (ObjOrErr)
+    return ObjOrErr->release();
+
+  outErr = std::string(path) +
+           ": Could not read LTO input file: " + toString(ObjOrErr.takeError());
+  return nullptr;
+}
+
+size_t LTOModule::getDependentLibraryCount(lto::InputFile *input) {
+  return input->getDependentLibraries().size();
+}
 
-  // Dependent Libraries
-  raw_string_ostream OSD(DependentLibraries);
-  if (NamedMDNode *DependentLibraries = getModule().getNamedMetadata("llvm.dependent-libraries"))
-    for (MDNode *N : DependentLibraries->operands())
-      OSD << " " << cast<MDString>(N->getOperand(0))->getString();
+const char *LTOModule::getDependentLibrary(lto::InputFile *input, size_t index,
+                                           size_t *size) {
+  StringRef S = input->getDependentLibraries()[index];
+  *size = S.size();
+  return S.data();
 }

llvm/trunk/test/LTO/X86/Inputs/list-dependent-libraries.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+
+!llvm.dependent-libraries = !{!0, !1, !0}
+
+!0 = !{!"foo"}
+!1 = !{!"b a r"}

llvm/trunk/test/LTO/X86/list-dependent-libraries.ll

+; RUN: rm -rf %t && mkdir -p %t
+; RUN: llvm-as -o %t/1.bc %s
+; RUN: llvm-as -o %t/2.bc %S/Inputs/list-dependent-libraries.ll
+; RUN: llvm-lto -list-dependent-libraries-only %t/1.bc %t/2.bc | FileCheck %s
+; REQUIRES: default_triple
+
+; CHECK: 1.bc:
+; CHECK-NEXT: wibble
+; CHECK: 2.bc:
+; CHECK-NEXT: foo
+; CHECK-NEXT: b a r
+; CHECK-NEXT: foo
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+
+!llvm.dependent-libraries = !{!0}
+
+!0 = !{!"wibble"}

llvm/trunk/test/tools/llvm-lto/error.ll

 ; RUN: not llvm-lto foobar 2>&1 | FileCheck %s
 ; CHECK: llvm-lto: error loading file 'foobar': {{N|n}}o such file or directory
 
 ; RUN: not llvm-lto --list-symbols-only %S/Inputs/empty.bc 2>&1 | FileCheck %s --check-prefix=CHECK-LIST
 ; CHECK-LIST: llvm-lto: error loading file '{{.*}}/Inputs/empty.bc': The file was not recognized as a valid object file
 
+; RUN: not llvm-lto --list-dependent-libraries-only %S/Inputs/empty.bc 2>&1 | FileCheck %s --check-prefix=CHECK-LIBS
+; CHECK-LIBS: llvm-lto: {{.*}}/Inputs/empty.bc: Could not read LTO input file: The file was not recognized as a valid object file
+
 ; RUN: not llvm-lto --thinlto %S/Inputs/empty.bc 2>&1 | FileCheck %s --check-prefix=CHECK-THIN
 ; CHECK-THIN: llvm-lto: error loading file '{{.*}}/Inputs/empty.bc': Invalid bitcode signature

llvm/trunk/tools/llvm-lto/llvm-lto.cpp

static cl::list<std::string>
     DSOSymbols("dso-symbol",
                cl::desc("Symbol to put in the symtab in the resulting dso"),
                cl::ZeroOrMore);
 
 static cl::opt<bool> ListSymbolsOnly(
     "list-symbols-only", cl::init(false),
     cl::desc("Instead of running LTO, list the symbols in each IR file"));
 
+static cl::opt<bool> ListDependentLibrariesOnly(
+    "list-dependent-libraries-only", cl::init(false),
+    cl::desc("Instead of running LTO, list the dependent libraries in each IR file"));
+
 static cl::opt<bool> SetMergedModule(
     "set-merged-module", cl::init(false),
     cl::desc("Use the first input module as the merged module"));
 
 static cl::opt<unsigned> Parallelism("j", cl::Prefix, cl::init(1),
                                      cl::desc("Number of backend threads"));
 
 static cl::opt<bool> RestoreGlobalsLinkage(
for (auto &Filename : InputFilenames) {
 
     // List the symbols.
     outs() << Filename << ":\n";
     for (int I = 0, E = Module->getSymbolCount(); I != E; ++I)
       outs() << Module->getSymbolName(I) << "\n";
   }
 }
 
+static std::unique_ptr<MemoryBuffer> loadFile(StringRef Filename) {
+    ExitOnError ExitOnErr("llvm-lto: error loading file '" + Filename.str() +
+        "': ");
+    return ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename)));
+}
+
+static void listDependentLibraries() {
+  for (auto &Filename : InputFilenames) {
+    auto Buffer = loadFile(Filename);
+    std::string E;
+    std::unique_ptr<lto::InputFile> Input(LTOModule::createInputFile(
+        Buffer->getBufferStart(), Buffer->getBufferSize(), Filename.c_str(),
+        E));
+    if (!Input)
+      error(E);
+
+    // List the dependent libraries.
+    outs() << Filename << ":\n";
+    for (size_t I = 0, C = LTOModule::getDependentLibraryCount(Input.get());
+         I != C; ++I) {
+      size_t L = 0;
+      const char *S = LTOModule::getDependentLibrary(Input.get(), I, &L);
+      assert(S);
+      outs() << StringRef(S, L) << "\n";
+    }
+  }
+}
+
 /// Create a combined index file from the input IR files and write it.
 ///
 /// This is meant to enable testing of ThinLTO combined index generation,
 /// currently available via the gold plugin via -thinlto.
 static void createCombinedModuleSummaryIndex() {
   ModuleSummaryIndex CombinedIndex(/*HaveGVs=*/false);
   uint64_t NextModuleId = 0;
   for (auto &Filename : InputFilenames) {

 std::unique_ptr<ModuleSummaryIndex> loadCombinedIndex() {
   if (ThinLTOIndex.empty())
     report_fatal_error("Missing -thinlto-index for ThinLTO promotion stage");
   ExitOnError ExitOnErr("llvm-lto: error loading file '" + ThinLTOIndex +
                         "': ");
   return ExitOnErr(getModuleSummaryIndexForFile(ThinLTOIndex));
 }
 
-static std::unique_ptr<MemoryBuffer> loadFile(StringRef Filename) {
-  ExitOnError ExitOnErr("llvm-lto: error loading file '" + Filename.str() +
-                        "': ");
-  return ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename)));
-}
-
 static std::unique_ptr<lto::InputFile> loadInputFile(MemoryBufferRef Buffer) {
   ExitOnError ExitOnErr("llvm-lto: error loading input '" +
                         Buffer.getBufferIdentifier().str() + "': ");
   return ExitOnErr(lto::InputFile::create(Buffer));
 }
 
 static std::unique_ptr<Module> loadModuleFromInput(lto::InputFile &File,
                                                    LLVMContext &CTX) {
int main(int argc, char **argv) {
   // set up the TargetOptions for the machine
   TargetOptions Options = InitTargetOptionsFromCodeGenFlags();
 
   if (ListSymbolsOnly) {
     listSymbols(Options);
     return 0;
   }
 
+  if (ListDependentLibrariesOnly) {
+    listDependentLibraries();
+    return 0;
+  }
+
   if (IndexStats) {
     printIndexStats();
     return 0;
   }
 
   if (CheckHasObjC) {
     for (auto &Filename : InputFilenames) {
       ExitOnError ExitOnErr(std::string(*argv) + ": error loading file '" +

llvm/trunk/tools/lto/lto.cpp

 
 #include "llvm-c/lto.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/CodeGen/CommandFlags.inc"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/LLVMContext.h"
+#include "llvm/LTO/LTO.h"
 #include "llvm/LTO/legacy/LTOCodeGenerator.h"
 #include "llvm/LTO/legacy/LTOModule.h"
 #include "llvm/LTO/legacy/ThinLTOCodeGenerator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
 
lto_symbol_attributes lto_module_get_symbol_attribute(lto_module_t mod,
                                                       unsigned int index) {
   return unwrap(mod)->getSymbolAttributes(index);
 }
 
 const char* lto_module_get_linkeropts(lto_module_t mod) {
   return unwrap(mod)->getLinkerOpts().data();
 }
 
-const char* lto_module_get_dependent_libraries(lto_module_t mod) {
-    return unwrap(mod)->getDependentLibraries().data();
-}
-
 void lto_codegen_set_diagnostic_handler(lto_code_gen_t cg,
                                         lto_diagnostic_handler_t diag_handler,
                                         void *ctxt) {
   unwrap(cg)->setDiagnosticHandler(diag_handler, ctxt);
 }
 
 static lto_code_gen_t createCodeGen(bool InLocalContext) {
   lto_initialize();
case LTO_CODEGEN_PIC_MODEL_DYNAMIC_NO_PIC:
     return false;
   case LTO_CODEGEN_PIC_MODEL_DEFAULT:
     unwrap(cg)->setCodePICModel(None);
     return false;
   }
   sLastErrorString = "Unknown PIC model";
   return true;
 }
+
+DEFINE_SIMPLE_CONVERSION_FUNCTIONS(lto::InputFile, lto_input_t)
+
+lto_input_t lto_input_create(const void *buffer, size_t buffer_size, const char *path) {
+  return wrap(LTOModule::createInputFile(buffer, buffer_size, path, sLastErrorString));
+}
+
+void lto_input_dispose(lto_input_t input) {
+  delete unwrap(input);
+}
+
+extern unsigned lto_input_get_num_dependent_libraries(lto_input_t input) {
+  return LTOModule::getDependentLibraryCount(unwrap(input));
+}
+
+extern const char *lto_input_get_dependent_library(lto_input_t input,
+                                                   size_t index,
+                                                   size_t *size) {
+  return LTOModule::getDependentLibrary(unwrap(input), index, size);
+}

llvm/trunk/tools/lto/lto.exports

 lto_module_is_thinlto
 thinlto_codegen_add_must_preserve_symbol
 thinlto_codegen_add_cross_referenced_symbol
 thinlto_codegen_set_codegen_only
 thinlto_codegen_disable_codegen
 thinlto_module_get_num_object_files
 thinlto_module_get_object_file
 thinlto_set_generated_objects_dir
+lto_input_create
+lto_input_destroy
+lto_input_get_num_dependent_libraries
+lto_input_get_dependent_library