This is an archive of the discontinued LLVM Phabricator instance.

[lld] Add support for other demanglers other than Itanium
ClosedPublic

Authored by ljmf00 on Dec 25 2021, 10:53 AM.

Details

Summary

LLVM core library supports demangling other mangled symbols other than itanium, such as D and Rust. LLD should use those demanglers in order to output pretty demangled symbols on error messages.

Diff Detail

Event Timeline

ljmf00 created this revision.Dec 25 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ljmf00 requested review of this revision.Dec 25 2021, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2021, 10:53 AM
ljmf00 updated this revision to Diff 396201.Dec 25 2021, 11:00 AM
ljmf00 updated this revision to Diff 396202.Dec 25 2021, 11:14 AM
ljmf00 edited the summary of this revision. (Show Details)Dec 25 2021, 11:16 AM
MaskRay added inline comments.Dec 25 2021, 11:59 AM
lld/Common/Strings.cpp
27

The tests are in case demangle(...) is slow on non-mangled symbols.

I have linked chrome with --dynamic-list specifying a extern "C" version node and don't see much difference, so this seems fine.

lld/ELF/Symbols.cpp
31

Prefer config whenever possible.

ljmf00 added inline comments.Dec 25 2021, 12:41 PM
lld/Common/Strings.cpp
27

What do you mean by this? Is this validation or asking to be kept as is? There are checks against this already on llvm::demangle. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Demangle/Demangle.cpp#L51 . This includes checks not only for Itanium encoding but also for D and Rust.

ljmf00 updated this revision to Diff 396208.Dec 25 2021, 1:03 PM
ljmf00 added inline comments.
lld/ELF/Symbols.cpp
31

Changed.

MaskRay added inline comments.Dec 25 2021, 1:12 PM
lld/Common/Strings.cpp
27

No action needed.

See SymbolTable::getDemangledSyms. When extern "C++" is used for ld.lld --dynamic-list, every symbol is demangled. A large executable may have millions of symbols (chrome), so the performance matters.

1559632 of 1571293 symbols in chrome's symbol table starts with _, so the check may be redundant.

lld/include/lld/Common/Strings.h
21

llvm-project/lld nearly doesn't use doxygen \param \returns. The original comment style is sufficiently clear. The new \param just adds information a user can easily infer.

ljmf00 updated this revision to Diff 396209.Dec 25 2021, 2:18 PM

I built and checked with x86 and experimental WASM target locally on a Linux environment.

lld/Common/Strings.cpp
27

Ok, thanks for clarifying. Yes, right now this check is restrictive and redundant.

lld/include/lld/Common/Strings.h
21

Reverted. Just removed the C++ restriction.

MaskRay accepted this revision as: MaskRay.Dec 28 2021, 1:49 PM

Looks great!

This revision is now accepted and ready to land.Dec 28 2021, 1:49 PM
This revision was automatically updated to reflect the committed changes.
ljmf00 reopened this revision.Dec 30 2021, 10:40 AM

I reverted this to not congestionante other builds and described the reason on the revert commit. I will do a clean build with all targets and try to reproduce this. I only tested this on x86 and WebAssembly targets since I haven't seen target specific code here. Additionally, I've tested this on an x86_64 environment.

This revision is now accepted and ready to land.Dec 30 2021, 10:40 AM

The failure before the revert was:

tools/lld/MachO/CMakeFiles/lldMachO.dir/Symbols.cpp.o: In function `lld::demangle(llvm::StringRef, bool)':

This affects common code and all ports may be affected. You can use ninja check-lld to test all ports. not just check-lld-wasm or check-lld-elf

The failure before the revert was:

tools/lld/MachO/CMakeFiles/lldMachO.dir/Symbols.cpp.o: In function `lld::demangle(llvm::StringRef, bool)':

This affects common code and all ports may be affected. You can use ninja check-lld to test all ports. not just check-lld-wasm or check-lld-elf

I've done it but only with "X86" on LLVM_TARGETS_TO_BUILD, since this can reduce build times. I've done a clean build with all targets and additionally the WebAssembly experimental target and the testsuite ran successfully with ninja check-lld. That said, I can't reproduce the issue in my environment. I can investigate further, but I don't have a PowerPC64 environment.

ljmf00 updated this revision to Diff 396705.Dec 30 2021, 12:09 PM

I finally reproduced this locally. The buildbot is configured with BUILD_SHARED_LIBS and my builds are built without it and use LLVM_LINK_LLVM_DYLIB and CLANG_LINK_CLANG_DYLIB respectively. Since I made lld::demangle symbol inline, Demangle LLVM component was missing on MachO. I added that component to the link list on CMakeLists.txt.

MaskRay added a comment.EditedDec 30 2021, 12:19 PM

If you don't contribute to llvm-project, LLVM_TARGETS_TO_BUILD=host or just X86 is fine. For contribution, it's a good idea to enable all targets, otherwise some tests may be skipped.

(The missing Demangle dependency issue)
A -DBUILD_SHARED_LIBS=on build uses -Wl,-z,defs to link shared objects. It has some dependency checking effects: https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs

Personally I use -DBUILD_SHARED_LIBS=on for Debug build and -DBUILD_SHARED_LIBS=off for Release build, and ensure I built both modes if I touch CMake files.

MaskRay accepted this revision.Dec 30 2021, 12:20 PM

If you don't contribute to llvm-project, LLVM_TARGETS_TO_BUILD=host or just X86 is fine. For contribution, it's a good idea to enable all targets, otherwise some tests may be skipped.

Yeah, I'm going to start doing that.

(The missing Demangle dependency issue)
A -DBUILD_SHARED_LIBS=on build uses -Wl,-z,defs to link shared objects. It has some dependency checking effects: https://maskray.me/blog/2021-06-13-dependency-related-linker-options#z-defs

Personally I use -DBUILD_SHARED_LIBS=on for Debug build and -DBUILD_SHARED_LIBS=off for Release build, and ensure I built both modes if I touch CMake files.

Thanks for all that information. I didn't know about that. I will start building with it :)

This revision was automatically updated to reflect the committed changes.