This is an archive of the discontinued LLVM Phabricator instance.

[NFC] WebAssembly build fix
ClosedPublic

Authored by jfb on May 16 2018, 2:25 PM.

Details

Summary

r332305 added a use of llvm::wasm::toString in llvm::object::WasmSymbol::print,
which is in a header file. It also moves toString to BinaryFormat. This has the
unintended side-effect that any inclusion of Object/Wasm.h now relies on
toString, and needs to required_libraries = BinaryFormat. Thankfully most builds
don't fail with this because print just isn't used and gets eliminated, dropping
the required dependency in the process. Not all builds are so lucky.

Fix this issue by moving print to the corresponding .cpp file.

rdar://problem/40258137

Diff Detail

Repository
rL LLVM

Event Timeline

jfb created this revision.May 16 2018, 2:25 PM
jfb accepted this revision.May 16 2018, 2:27 PM

Seems like a no-brainer, I'll commit.

This revision is now accepted and ready to land.May 16 2018, 2:27 PM
This revision was automatically updated to reflect the committed changes.

LGTM, thanks

Thanks. Makes sense.

My rational with the original change is that anyone who depends on libObject is almost certainly also depending on libBinaryFormat (at the very least the headers). But your change makes thing nicer since it allows some users to avoid the explicit dependency on the lib I guess.

vsk added a subscriber: vsk.May 16 2018, 3:25 PM

I still see a build break with LLVM_ENABLE_MODULES=On:

$ ninja DebugInfoCodeViewTests llvm-rc       
[1/2] Linking CXX executable bin/llvm-rc
FAILED: bin/llvm-rc 
: && /Volumes/Xcode10A177_m18A288_i16A283_t16J278_w16R278a_b16P290a_XcodeInternals_32bitSupport_FastSim_Boost_ASan_36GB/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/Users/vsk/src/builds/llvm.org-master-RA/module.cache -fcxx-modules -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip tools/llvm-rc/CMakeFiles/llvm-rc.dir/llvm-rc.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceFileWriter.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptCppFilter.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptParser.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptStmt.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptToken.cpp.o  -o bin/llvm-rc  -Wl,-rpath,@loader_path/../lib lib/libLLVMOption.a lib/libLLVMSupport.a lib/libLLVMBinaryFormat.a lib/libLLVMSupport.a -lz -lcurses -lm lib/libLLVMDemangle.a && :
Undefined symbols for architecture x86_64:
  "llvm::object::WasmSymbol::print(llvm::raw_ostream&) const", referenced from:
      llvm::object::WasmSymbol::dump() const in ResourceFileWriter.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2/2] Linking CXX executable unittests/DebugInfo/CodeView/DebugInfoCodeViewTests
FAILED: unittests/DebugInfo/CodeView/DebugInfoCodeViewTests 
: && /Volumes/Xcode10A177_m18A288_i16A283_t16J278_w16R278a_b16P290a_XcodeInternals_32bitSupport_FastSim_Boost_ASan_36GB/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/Users/vsk/src/builds/llvm.org-master-RA/module.cache -fcxx-modules -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/RandomAccessVisitorTest.cpp.o unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/TypeHashingTest.cpp.o unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/TypeIndexDiscoveryTest.cpp.o  -o unittests/DebugInfo/CodeView/DebugInfoCodeViewTests  lib/libLLVMDebugInfoCodeView.a lib/libLLVMBinaryFormat.a lib/libLLVMSupport.a lib/libgtest_main.a lib/libgtest.a lib/libLLVMTestingSupport.a lib/libLLVMDebugInfoMSF.a lib/libgtest.a lib/libLLVMSupport.a -lz -lcurses -lm lib/libLLVMDemangle.a -lpthread && :
Undefined symbols for architecture x86_64:
  "llvm::object::WasmSymbol::print(llvm::raw_ostream&) const", referenced from:
      llvm::object::WasmSymbol::dump() const in RandomAccessVisitorTest.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Do we need to make these targets depend on libObject?

jfb added a comment.May 16 2018, 3:25 PM

Thanks. Makes sense.

My rational with the original change is that anyone who depends on libObject is almost certainly also depending on libBinaryFormat (at the very least the headers). But your change makes thing nicer since it allows some users to avoid the explicit dependency on the lib I guess.

Also: compile-time. Anything I don't intend to get inlined I'd rather have in .cpp files.

jfb added a comment.May 16 2018, 3:27 PM
In D46977#1102147, @vsk wrote:

I still see a build break with LLVM_ENABLE_MODULES=On:

$ ninja DebugInfoCodeViewTests llvm-rc       
[1/2] Linking CXX executable bin/llvm-rc
FAILED: bin/llvm-rc 
: && /Volumes/Xcode10A177_m18A288_i16A283_t16J278_w16R278a_b16P290a_XcodeInternals_32bitSupport_FastSim_Boost_ASan_36GB/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/Users/vsk/src/builds/llvm.org-master-RA/module.cache -fcxx-modules -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip tools/llvm-rc/CMakeFiles/llvm-rc.dir/llvm-rc.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceFileWriter.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptCppFilter.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptParser.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptStmt.cpp.o tools/llvm-rc/CMakeFiles/llvm-rc.dir/ResourceScriptToken.cpp.o  -o bin/llvm-rc  -Wl,-rpath,@loader_path/../lib lib/libLLVMOption.a lib/libLLVMSupport.a lib/libLLVMBinaryFormat.a lib/libLLVMSupport.a -lz -lcurses -lm lib/libLLVMDemangle.a && :
Undefined symbols for architecture x86_64:
  "llvm::object::WasmSymbol::print(llvm::raw_ostream&) const", referenced from:
      llvm::object::WasmSymbol::dump() const in ResourceFileWriter.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2/2] Linking CXX executable unittests/DebugInfo/CodeView/DebugInfoCodeViewTests
FAILED: unittests/DebugInfo/CodeView/DebugInfoCodeViewTests 
: && /Volumes/Xcode10A177_m18A288_i16A283_t16J278_w16R278a_b16P290a_XcodeInternals_32bitSupport_FastSim_Boost_ASan_36GB/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/Users/vsk/src/builds/llvm.org-master-RA/module.cache -fcxx-modules -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fcolor-diagnostics -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -Wl,-dead_strip unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/RandomAccessVisitorTest.cpp.o unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/TypeHashingTest.cpp.o unittests/DebugInfo/CodeView/CMakeFiles/DebugInfoCodeViewTests.dir/TypeIndexDiscoveryTest.cpp.o  -o unittests/DebugInfo/CodeView/DebugInfoCodeViewTests  lib/libLLVMDebugInfoCodeView.a lib/libLLVMBinaryFormat.a lib/libLLVMSupport.a lib/libgtest_main.a lib/libgtest.a lib/libLLVMTestingSupport.a lib/libLLVMDebugInfoMSF.a lib/libgtest.a lib/libLLVMSupport.a -lz -lcurses -lm lib/libLLVMDemangle.a -lpthread && :
Undefined symbols for architecture x86_64:
  "llvm::object::WasmSymbol::print(llvm::raw_ostream&) const", referenced from:
      llvm::object::WasmSymbol::dump() const in RandomAccessVisitorTest.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Do we need to make these targets depend on libObject?

I'm not sure we want to. Let me fix it by moving dump as well.

jfb added a comment.May 16 2018, 3:31 PM
In D46977#1102150, @jfb wrote:
In D46977#1102147, @vsk wrote:

I still see a build break with LLVM_ENABLE_MODULES=On:

Do we need to make these targets depend on libObject?

I'm not sure we want to. Let me fix it by moving dump as well.

https://reviews.llvm.org/D46985