[llvm-libtool-darwin] Fix crash with bitcode asm module

[keith]

Summary

When using llvm-libtool-darwin with LTO building llvm itself, it crashed
on a file with an asm module in the bitcode. This fixes that by
correctly registering the targets for this.

[keith]

Curious to hear if this is the right approach. Registering all of these targets seems like a big hammer but I think it's required unless we want to single out the targets darwin uses?

[keith]

Maybe we should actually be doing what llvm-ar is? https://github.com/llvm/llvm-project/blob/a6298fb160633578f5c4c1e739044274ba8fe520/llvm/tools/llvm-ar/llvm-ar.cpp#L1271-L1273

[keith]

clang-format

[jhenderson]

I'm afraid I don't have any particular insight into this area, so this needs someone else to review.

[smeenai]

I don't have direct experience with this either, but this seems sensible (I assume InitializeAllTargets is a pre-req for InitializeAllAsmParsers, although llvm-ar is just using InitializeAllTargetInfos). I'll let it sit for a few days to give others the chance to chime in though.

CC @int3, who set up LTO for LLD for Mach-O.

llvm/test/tools/llvm-libtool-darwin/asm.test

1 ↗

(On Diff #404412)

I think you'll want REQUIRES lines for the targets you're using here (x86 and aarch64), to make the test not fail when LLVM is built without those targets. (I think just testing one architecture would be fine as well, to not require multiple targets to be enabled for running the test).

[keith]

Update test REQUIRES, use llvm-ar logic instead

[keith]

I've updated to just use the llvm-ar logic since I assume that's the most correct option.

llvm/test/tools/llvm-libtool-darwin/asm.test

1 ↗

(On Diff #404412)

Ah yes thanks, I added both here. The reason I want both is because theoretically depending on how you register the targets, it may only work for your native target, and not any other targets for cross compilation, so having both here verifies that. If you think having both requires is too strict, I can split these into 2 test files?

Also note apparently in llvm/test the lit config is slightly different so the requires definitions are different here.

[smeenai]

LGTM since we're just matching llvm-ar's logic.

llvm/test/tools/llvm-libtool-darwin/asm.test

1 ↗

(On Diff #404412)

Yeah, that logic for testing both makes sense. I do think it'd be nice to split out into two files though (just to maximize testing coverage).

[keith]

Split up test files

llvm/test/tools/llvm-libtool-darwin/Inputs/arm64-asm.ll

+target triple = "arm64-apple-macosx11.0.0"
+
+module asm ".desc ___crashreporter_info__, 0x10"
+
+define void @somesymbol() {
+  ret void
+}

llvm/test/tools/llvm-libtool-darwin/Inputs/x86_64-asm.ll

+target triple = "x86_64-apple-macosx11.0.0"
+
+module asm ".desc ___crashreporter_info__, 0x10"
+
+define void @somesymbol() {
+  ret void
+}

llvm/test/tools/llvm-libtool-darwin/arm64-asm.test

+# REQUIRES: aarch64-registered-target
+## This tests that archives are correctly created when the llvm
+## has native assembly info
+
+# RUN: llvm-as %p/Inputs/arm64-asm.ll -o %t-arm64-asm.bc
+# RUN: llvm-libtool-darwin -static -o %t-arm64.lib %t-arm64-asm.bc
+# RUN: llvm-nm %t-arm64.lib | FileCheck %s
+
+# CHECK: T somesymbol

llvm/test/tools/llvm-libtool-darwin/x86_64-asm.test

+# REQUIRES: x86-registered-target
+## This tests that archives are correctly created when the llvm
+## has native assembly info
+
+# RUN: llvm-as %p/Inputs/x86_64-asm.ll -o %t-x86_64-asm.bc
+# RUN: llvm-libtool-darwin -static -o %t-x86_64.lib %t-x86_64-asm.bc
+# RUN: llvm-nm %t-x86_64.lib | FileCheck %s
+
+# CHECK: T somesymbol

llvm/tools/llvm-libtool-darwin/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BinaryFormat
   Core
   Object
   Support
   TextAPI
+  ${LLVM_TARGETS_TO_BUILD}
   )
 
 add_llvm_tool(llvm-libtool-darwin
   llvm-libtool-darwin.cpp
 )
 
 if(LLVM_INSTALL_CCTOOLS_SYMLINKS)
   add_llvm_tool_symlink(libtool llvm-libtool-darwin)
 endif()

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/MachOUniversalWriter.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/LineIterator.h"
+#include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TextAPI/Architecture.h"
 #include <map>
 #include <type_traits>
 
 using namespace llvm;
int main(int Argc, char **Argv) {
   if (!ConfigOrErr) {
     WithColor::defaultErrorHandler(ConfigOrErr.takeError());
     return EXIT_FAILURE;
   }
 
   if (VersionOption)
     cl::PrintVersionMessage();
 
+  llvm::InitializeAllTargetInfos();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmParsers();
+
   Config C = *ConfigOrErr;
   switch (LibraryOperation) {
   case Operation::None:
     break;
   case Operation::Static:
     if (Error E = createStaticLibrary(C)) {
       WithColor::defaultErrorHandler(std::move(E));
       return EXIT_FAILURE;
     }
     break;
   }
 }