This is an archive of the discontinued LLVM Phabricator instance.

Differential D46977

[NFC] WebAssembly build fix
ClosedPublic

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

Details

Reviewers
sbc100
ncw
paquette
jfb
Commits
rG659932b0b2c7: [NFC] WebAssembly build fix
rL332530: [NFC] WebAssembly build fix
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
Herald added subscribers: llvm-commits, sunfish, aheejin and 2 others. · View Herald TranscriptMay 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
Harbormaster completed remote builds in B18212: Diff 147180.May 16 2018, 2:28 PM
Closed by commit rL332530: [NFC] WebAssembly build fix (authored by jfb). · Explain WhyMay 16 2018, 2:28 PM
This revision was automatically updated to reflect the committed changes.
dschuff added a comment.May 16 2018, 2:30 PM

LGTM, thanks

sbc100 added a comment.May 16 2018, 3:20 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.

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
In D46977#1102107, @sbc100 wrote:

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

jfb added a child revision: D46985: [NFC] WebAssembly build break #2.May 16 2018, 3:36 PM

Revision Contents

PathSize
llvm/
trunk/
include/
llvm/
Object/
13 lines
lib/
Object/
13 lines

Diff 147181

llvm/trunk/include/llvm/Object/Wasm.h

Show First 20 Lines • Show All 82 Lines • ▼ Show 20 Linespublic:
bool isHidden() const { bool isHidden() const {
return getVisibility() == wasm::WASM_SYMBOL_VISIBILITY_HIDDEN; return getVisibility() == wasm::WASM_SYMBOL_VISIBILITY_HIDDEN;
} }
unsigned getVisibility() const { unsigned getVisibility() const {
return Info.Flags & wasm::WASM_SYMBOL_VISIBILITY_MASK; return Info.Flags & wasm::WASM_SYMBOL_VISIBILITY_MASK;
} }
void print(raw_ostream &Out) const { void print(raw_ostream &Out) const;
Out << "Name=" << Info.Name
<< ", Kind=" << toString(wasm::WasmSymbolType(Info.Kind))
<< ", Flags=" << Info.Flags;
if (!isTypeData()) {
Out << ", ElemIndex=" << Info.ElementIndex;
} else if (isDefined()) {
Out << ", Segment=" << Info.DataRef.Segment;
Out << ", Offset=" << Info.DataRef.Offset;
Out << ", Size=" << Info.DataRef.Size;
}
}
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD void dump() const { print(dbgs()); } LLVM_DUMP_METHOD void dump() const { print(dbgs()); }
#endif #endif
}; };
struct WasmSection { struct WasmSection {
WasmSection() = default; WasmSection() = default;
▲ Show 20 Lines • Show All 172 Lines • Show Last 20 Lines

llvm/trunk/lib/Object/WasmObjectFile.cpp

Show All 29 Lines
#include <cstring> #include <cstring>
#include <system_error> #include <system_error>
#define DEBUG_TYPE "wasm-object" #define DEBUG_TYPE "wasm-object"
using namespace llvm; using namespace llvm;
using namespace object; using namespace object;
void WasmSymbol::print(raw_ostream &Out) const {
Out << "Name=" << Info.Name
<< ", Kind=" << toString(wasm::WasmSymbolType(Info.Kind))
<< ", Flags=" << Info.Flags;
if (!isTypeData()) {
Out << ", ElemIndex=" << Info.ElementIndex;
} else if (isDefined()) {
Out << ", Segment=" << Info.DataRef.Segment;
Out << ", Offset=" << Info.DataRef.Offset;
Out << ", Size=" << Info.DataRef.Size;
}
}
Expected<std::unique_ptr<WasmObjectFile>> Expected<std::unique_ptr<WasmObjectFile>>
ObjectFile::createWasmObjectFile(MemoryBufferRef Buffer) { ObjectFile::createWasmObjectFile(MemoryBufferRef Buffer) {
Error Err = Error::success(); Error Err = Error::success();
auto ObjectFile = llvm::make_unique<WasmObjectFile>(Buffer, Err); auto ObjectFile = llvm::make_unique<WasmObjectFile>(Buffer, Err);
if (Err) if (Err)
return std::move(Err); return std::move(Err);
return std::move(ObjectFile); return std::move(ObjectFile);
▲ Show 20 Lines • Show All 1,226 Lines • Show Last 20 Lines