BugPoint tests are failing when compiled with LTO

[petrhosek]

Bugzilla Link	51230
Version	unspecified
OS	All
CC	@gkmhub,@MaskRay,@int3,@thevinster,@nico,@smeenai,@oontvoo

Extended Description

I tried enabling the use of ld64.lld in our toolchain build that uses (regular) LTO but I see a number of BugPoint tests failure which all looks the same:

Error opening '/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/BugpointPasses.dylib': dlopen(/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/BugpointPasses.dylib, 9): Symbol not found: __ZNK4llvm4Pass11getPassNameEv
  Referenced from: /opt/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/BugpointPasses.dylib
  Expected in: flat namespace
 in /opt/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/lib/BugpointPasses.dylib
  -load request ignored.
bugpoint: Unknown command line argument '-bugpoint-crashcalls'.  Try: '/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/bin/bugpoint --help'
bugpoint: Did you mean '--print-after-all'?

I suspect it's related to the use of -flat_namespace, see

llvm-project/llvm/cmake/modules/HandleLLVMOptions.cmake

Line 174 in e69a8c4

set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-flat_namespace -Wl,-undefined -Wl,suppress")

, which should be supported under LTO according to

llvm-project/lld/MachO/LTO.cpp

Line 83 in d7d2e45

config->namespaceKind == NamespaceKind::flat ||

but perhaps it doesn't behave the same way as ld64?

[petrhosek]

I tested ThinLTO as well for comparison and this issue isn't manifesting so it appears to be related to full LTO.

[petrhosek]

One clarification, this affects other tests besides BugPoint. The full list of failing tests is:

LLVM :: BugPoint/attr-crash.ll
LLVM :: BugPoint/crash-narrowfunctiontest.ll
LLVM :: BugPoint/func-attrs-keyval.ll
LLVM :: BugPoint/func-attrs.ll
LLVM :: BugPoint/invalid-debuginfo.ll
LLVM :: BugPoint/metadata.ll
LLVM :: BugPoint/named-md.ll
LLVM :: BugPoint/remove_arguments_test.ll
LLVM :: BugPoint/replace-funcs-with-null.ll
LLVM :: BugPoint/retain-crashing-metadata.ll
LLVM :: BugPoint/unsymbolized.ll
LLVM :: ExecutionEngine/OrcLazy/debug-descriptor-elf-minimal.ll
LLVM :: ExecutionEngine/OrcLazy/debug-objects-elf-minimal.ll
LLVM-Unit :: Passes/./PluginsTests/PluginsTests.LoadMultiplePlugins
LLVM-Unit :: Passes/./PluginsTests/PluginsTests.LoadPlugin
LLVM-Unit :: Support/DynamicLibrary/./DynamicLibraryTests/DynamicLibrary.Overload

This seems to be related to the use of dynamic plugins.

[petrhosek]

To reproduce this issue, I used the following steps:

$ mkdir /Users/phosek/llvm/build/stage1 && cd /Users/phosek/llvm/build/stage1
$ cmake -GNinja
-DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_ASSERTIONS=ON
-DLLVM_ENABLE_PROJECTS="clang;lld;llvm"
../../llvm-project/llvm
$ ninja clang lld LTO llvm-symbolizer
$ mkdir /Users/phosek/llvm/build/stage2 && cd /Users/phosek/llvm/build/stage2
$ cmake -GNinja
-DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_ASSERTIONS=ON
-DCMAKE_C_COMPILER=/Users/phosek/llvm/build/stage1/bin/clang
-DCMAKE_CXX_COMPILER=/Users/phosek/llvm/build/stage1/bin/clang++
-DLLVM_ENABLE_PROJECTS="clang;lld;llvm"
-DLLVM_ENABLE_LLD=ON
-DLLVM_ENABLE_LTO=ON
-DDARWIN_LTO_LIBRARY=/Users/phosek/llvm/build/stage1/lib/libLTO.dylib
-DDYLD_LIBRARY_PATH=/Users/phosek/llvm/build/stage1/lib
../../llvm-project/llvm
$ ninja check-llvm

[smeenai]

Sorry for the slow responses here.

LLD for Mach-O doesn't support -export_dynamic for LTO yet (

llvm-project/lld/MachO/Driver.cpp

Lines 525 to 531 in d1fdd74

	// FIXME: Remove this once LTO.cpp honors config->exportDynamic.
	if (config->exportDynamic)
	for (InputFile *file : inputFiles)
	if (isa<BitcodeFile>(file)) {
	warn("the effect of -export_dynamic on LTO is not yet implemented");
	break;
	}

), which I believe to be the root cause of these issues. Not sure why ThinLTO would still work.

I'm looking into it.

[smeenai]

I think this is more of an issue with how we're setting VisibleToRegularObj.

From what I can tell, VisibleToRegularObj roughly tells LTO to not remove symbols even if it thinks they're unreferenced (among other things). There's also ExportDynamic in the resolution, but as far as I can tell, that's only used for whole program vtable analysis.

We currently set VisibleToRegularObj as follows:

r.VisibleToRegularObj = config->outputType != MH_EXECUTE ||
                        config->namespaceKind == NamespaceKind::flat ||
                        sym->isUsedInRegularObj;

We could fix this particular problem by doing something like

(config->outputType != MH_EXECUTE || config->exportDynamic)

instead. However, this might also be a good opportunity to try to more thoroughly populate our LTO resolutions.

Even for the dynamic library or -export_dynamic case, we should only need to set VisibleToRegularObj on symbols that aren't private_extern (the Mach-O equivalent of ELF's hidden visibility). We only store the privateExtern bit on Defined symbols though, so we'd either have to move that bit to the base Symbol class or do some casting. (It only makes sense on Defined symbols, so I think casting is the way to go.)

Setting ExportDynamic correctly in the resolution should be pretty straightforward: it's true if a symbol isn't private_extern and either you're not building an executable or you're building with -export_dynamic. It'll also require the privateExtern symbol bit to be available though.

I don't understand why our current VisibleToRegularObj considers -flat_namespace. This was added in https://reviews.llvm.org/D99105; Jez, do you remember the motivation? (I see an explanation in the commit message but I don't think I'm understanding it correctly.) Note that the actual test executable is not built with -flat_namespace (and that's where the problem is occurring); it's only the plugin dylib that's built with -flat_namespace.

CCing Fangrui and Teresa to confirm my understanding of what VisibleToRegularObj and ExportDynamic means for an LTO symbol resolution. Also, does it make a difference if we only set those for prevailing symbols? (It'd make our code slightly nicer, but I don't know if it's correct to do.)

[int3]

I don't understand why our current VisibleToRegularObj considers -flat_namespace.

I'll be honest, looking at that diff, I'm not sure either... I think I may have just misunderstood flat_namespace back then :/

[int3]

I have confirmed that https://reviews.llvm.org/D119372 fixes this.