Page MenuHomePhabricator

clang+lld: Improve clang+ld.darwinnew.lld interaction, pass -demangle
ClosedPublic

Authored by thakis on Fri, Nov 20, 11:06 AM.

Details

Summary

This patch:

  • adds an ld64.lld.darwinnew symlink for lld, to go with f2710d4b576, so that clang -fuse-ld=lld.darwinnew can be used to test new Mach-O lld while it's in bring-up. (The expectation is that we'll remove this again once new Mach-O lld is the defauld and only Mach-O lld.)
  • lets the clang driver know if the linker is lld (currently only triggered if -fuse-ld=lld or -fuse-ld=lld.darwinnew is passed). Currently only used for the next point, but could be used to implement other features that need close coordination between compiler and linker, e.g. having a diag for calling clang++ instead of clang when link errors are caused by a missing C++ stdlib.
  • lets the clang driver pass -demangle to Mach-O lld (both old and new), in addition to ld64
  • implementes -demangle for new Mach-O lld
  • changes demangleItanium() to accept _Z, Z, _Z, ____Z prefixes (and removes one test added in D68014 that doesnt' seem all that useful). Mach-O has an extra underscore for symbols, and the three (or, on Mach-O, four) underscores are used for block names.

Diff Detail

Event Timeline

thakis created this revision.Fri, Nov 20, 11:06 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
thakis requested review of this revision.Fri, Nov 20, 11:06 AM
MaskRay added inline comments.Fri, Nov 20, 11:30 AM
lld/test/ELF/undef.s
88

ELF does not demangle __Z prefixed symbols. Removal of a test does not seem right.

thakis added inline comments.Fri, Nov 20, 1:47 PM
lld/test/ELF/undef.s
88

Yes, but when is this ever going to be hit in practice?

The alternative is to pass an "extra underscore" bit to itaniumDemangle() and set that differently for darwin and linux. That seems like overhead with close to 0 benefit, and llvm/lib/Demangle/Demangle.cpp made the same tradeoff.

hans added inline comments.Mon, Nov 23, 5:17 AM
clang/include/clang/Driver/ToolChain.h
331

nit: iis

clang/lib/Driver/ToolChain.cpp
609

It seems nicer for the caller if *LinkerIsLLD is always set to either true or false, so the caller doesn't have to initialize it.

For example:

if (LinkerIsLLD)
  *LinkerIsLLD = (UseLinker == "lld" || UseLinker == "lld.darwinnew");
clang/lib/Driver/ToolChains/Darwin.cpp
696

Why is this removed?

lld/Common/Strings.cpp
24–28

Does the comment above need updating now that this is checking not just for "_Z", but also multiple leading underscores?
Actually I found the comment pretty confusing in its current state too.
The idea here is to only call the demangler for C++ symbols, right? Maybe the comment could just say that.

lld/MachO/Options.td
1109–1110

All options seem to belong to a Group and also be ordered in the .td file according to group. Now that it's no longer in grp_undocumented, maybe it should move into another group?

thakis marked an inline comment as done.

commens

Thanks!

clang/lib/Driver/ToolChain.cpp
609

If we guarantee to set it to false, we need to initialize it on all paths (ie set it to false at the start of the function). But done.

clang/lib/Driver/ToolChains/Darwin.cpp
696

I moved it up to line 540, since I need the bool that this now sets up there.

lld/Common/Strings.cpp
24–28

Removed some bits, added some bits, added an example.

lld/MachO/Options.td
1109–1110

Options without explicit end up in "OPTIONS:" in lld -flavor darwinnew --help, which seems the correct group for this flag. Do you think it should be in a different group? A new group?

hans accepted this revision.Mon, Nov 23, 6:01 AM

lgtm, probably check with maskray that he's happy first though.

lld/Common/Strings.cpp
24–28

Much nicer, thanks!

lld/MachO/Options.td
1109–1110

Are there any other options that are not in Group?
Perhaps grp_rare would fit? It seems like a bit of a catch-all, e.g. -v is there.

In any case, the .td file seems quite strictly organized around these groups, so if we take this out of grp_undocumented I think definition should at least move out of this part of the file -- either to a different group, or somewhere around the top/bottom if it doesn't get any group.

This revision is now accepted and ready to land.Mon, Nov 23, 6:01 AM

Thanks! I'll land this and follow up on eventual maskray feedback in a separate change. (Original comment was 24 min after upload, I replied well within west coast working hours, so maybe it's fine as-is.)

lld/MachO/Options.td
1109–1110

--color-diagnostics and --help, but these aren't in ld64 (and the former was added by me). But making these flags show up in "OPTIONS" seems correct to me. I guess I'll see what int3 says on https://reviews.llvm.org/D91894 for fatal_warnings and then I can follow up here to make demangle do the same thing.

Will wait until EOD to see if maskray is happy.

MaskRay added inline comments.Mon, Nov 23, 2:27 PM
clang/lib/Driver/ToolChain.cpp
551–554

bool *

lld/Common/Strings.cpp
24–28

The comment should be fixed.

Itanium mangling scheme does not allow __Z/___Z. They are Mach-O extensions and probably also COFF i386's (where symbols start with an additional _)

lld/test/ELF/undef.s
85–86

and removes one test added in D68014 that doesnt' seem all that useful

Can you keep the symbol and alter the error message instead? Keeping it has little maintenance burden but adds some coverage.

Will wait until EOD to see if maskray is happy.

Don't you want to get an ack from @int3 as well:)

thakis marked 2 inline comments as done.

maskray

Thanks!

I think the lld bits of this are small enough to qualify for post-commit review.

lld/Common/Strings.cpp
24–28

I think that's consistent with the new comment, no? Darwin prefixes all symbols with an extra underscore (including C), and after stripping that leading underscore the names are regular itanium names.

(It's not just Darwin btw: gcc on normal linux also adds that underscore with -fleading-underscore, for example.)

Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 24, 5:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript