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

[thakis]

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.

[MaskRay]

lld/test/ELF/undef.s

88

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

[thakis]

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]

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]

commens

[thakis]

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]

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.

[thakis]

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.

[thakis]

Will wait until EOD to see if maskray is happy.

[MaskRay]

clang/lib/Driver/ToolChain.cpp

551

bool *

lld/Common/Strings.cpp

24

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

83

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.

[MaskRay]

In D91884#2411336, @thakis wrote:

Will wait until EOD to see if maskray is happy.

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

[thakis]

maskray

[thakis]

Thanks!

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

lld/Common/Strings.cpp

24

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.)

[gkm]

Thank you for this!

clang/include/clang/Driver/ToolChain.h

public:
 
   // Helper methods
 
   std::string GetFilePath(const char *Name) const;
   std::string GetProgramPath(const char *Name) const;
 
   /// Returns the linker path, respecting the -fuse-ld= argument to determine
   /// the linker suffix or name.
-  std::string GetLinkerPath() const;
+  /// If LinkerIsLLD is non-nullptr, it is set to true if the returned linker
+  /// is LLD. If it's set, it can be assumed that the linker is LLD built
+  /// at the same revision as clang, and clang can make assumptions about
+  /// LLD's supported flags, error output, etc.
+  std::string GetLinkerPath(bool *LinkerIsLLD = nullptr) const;
 
   /// Returns the linker path for emitting a static library.
   std::string GetStaticLibToolPath() const;
 
   /// Dispatch to the specific toolchain for verbose printing.
   ///
   /// This is used when handling the verbose option to print detailed,
   /// toolchain-specific information useful for understanding the behavior of

clang/lib/Driver/ToolChain.cpp

 std::string ToolChain::GetFilePath(const char *Name) const {
   return D.GetFilePath(Name, *this);
 }
 
 std::string ToolChain::GetProgramPath(const char *Name) const {
   return D.GetProgramPath(Name, *this);
 }
 
-std::string ToolChain::GetLinkerPath() const {
+std::string ToolChain::GetLinkerPath(bool* LinkerIsLLD) const {
+  if (LinkerIsLLD)
+    *LinkerIsLLD = false;
+
   // Get -fuse-ld= first to prevent -Wunused-command-line-argument. -fuse-ld= is
   // considered as the linker flavor, e.g. "bfd", "gold", or "lld".
   const Arg* A = Args.getLastArg(options::OPT_fuse_ld_EQ);
   StringRef UseLinker = A ? A->getValue() : CLANG_DEFAULT_LINKER;
 
   // --ld-path= takes precedence over -fuse-ld= and specifies the executable
   // name. -B, COMPILER_PATH and PATH and consulted if the value does not
   // contain a path component separator.
if (llvm::sys::path::is_absolute(UseLinker)) {
     llvm::SmallString<8> LinkerName;
     if (Triple.isOSDarwin())
       LinkerName.append("ld64.");
     else
       LinkerName.append("ld.");
     LinkerName.append(UseLinker);
 
     std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-    if (llvm::sys::fs::can_execute(LinkerPath))
+    if (llvm::sys::fs::can_execute(LinkerPath)) {
+      if (LinkerIsLLD)
+        // FIXME: Remove lld.darwinnew here once it's the only MachO lld.
+        *LinkerIsLLD = UseLinker == "lld" || UseLinker == "lld.darwinnew";
       return LinkerPath;
     }
+  }
 
   if (A)
     getDriver().Diag(diag::err_drv_invalid_linker_name) << A->getAsString(Args);
 
   return GetProgramPath(getDefaultLinker());
 }
 
 std::string ToolChain::GetStaticLibToolPath() const {

clang/lib/Driver/ToolChains/Darwin.h

void ConstructJob(Compilation &C, const JobAction &JA,
                     const llvm::opt::ArgList &TCArgs,
                     const char *LinkingOutput) const override;
 };
 
 class LLVM_LIBRARY_VISIBILITY Linker : public MachOTool {
   bool NeedsTempPath(const InputInfoList &Inputs) const;
   void AddLinkArgs(Compilation &C, const llvm::opt::ArgList &Args,
                    llvm::opt::ArgStringList &CmdArgs,
-                   const InputInfoList &Inputs, unsigned Version[5]) const;
+                   const InputInfoList &Inputs, unsigned Version[5],
+                   bool LinkerIsLLD) const;
 
 public:
   Linker(const ToolChain &TC) : MachOTool("darwin::Linker", "linker", TC) {}
 
   bool hasIntegratedCPP() const override { return false; }
   bool isLinkJob() const override { return true; }
 
   void ConstructJob(Compilation &C, const JobAction &JA,

clang/lib/Driver/ToolChains/Darwin.cpp

static bool shouldLinkerNotDedup(bool IsLinkerOnlyAction, const ArgList &Args) {
   if (!IsLinkerOnlyAction) // Implicit -O0 for compile+linker only.
     return true;
   return false;
 }
 
 void darwin::Linker::AddLinkArgs(Compilation &C, const ArgList &Args,
                                  ArgStringList &CmdArgs,
                                  const InputInfoList &Inputs,
-                                 unsigned Version[5]) const {
+                                 unsigned Version[5], bool LinkerIsLLD) const {
   const Driver &D = getToolChain().getDriver();
   const toolchains::MachO &MachOTC = getMachOToolChain();
 
   // Newer linkers support -demangle. Pass it if supported and not disabled by
   // the user.
-  if (Version[0] >= 100 && !Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
+  if ((Version[0] >= 100 || LinkerIsLLD) &&
+      !Args.hasArg(options::OPT_Z_Xlinker__no_demangle))
     CmdArgs.push_back("-demangle");
 
+  // FIXME: Pass most of the flags below that check Version if LinkerIsLLD too.
+
   if (Args.hasArg(options::OPT_rdynamic) && Version[0] >= 137)
     CmdArgs.push_back("-export_dynamic");
 
   // If we are using App Extension restrictions, pass a flag to the linker
   // telling it that the compiled code has been audited.
   if (Args.hasFlag(options::OPT_fapplication_extension,
                    options::OPT_fno_application_extension, false))
     CmdArgs.push_back("-application_extension");
void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   unsigned Version[5] = {0, 0, 0, 0, 0};
   if (Arg *A = Args.getLastArg(options::OPT_mlinker_version_EQ)) {
     if (!Driver::GetReleaseVersion(A->getValue(), Version))
       getToolChain().getDriver().Diag(diag::err_drv_invalid_version_number)
           << A->getAsString(Args);
   }
 
+  bool LinkerIsLLD = false;
+  const char *Exec =
+      Args.MakeArgString(getToolChain().GetLinkerPath(&LinkerIsLLD));
+
   // I'm not sure why this particular decomposition exists in gcc, but
   // we follow suite for ease of comparison.
-  AddLinkArgs(C, Args, CmdArgs, Inputs, Version);
+  AddLinkArgs(C, Args, CmdArgs, Inputs, Version, LinkerIsLLD);
 
   if (willEmitRemarks(Args) &&
       checkRemarksOptions(getToolChain().getDriver(), Args,
                           getToolChain().getTriple()))
     renderRemarksOptions(Args, CmdArgs, getToolChain().getTriple(), Output, JA);
 
   // Propagate the -moutline flag to the linker in LTO.
   if (Arg *A =
void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
 
   ResponseFileSupport ResponseSupport = ResponseFileSupport::AtFileUTF8();
   if (Version[0] < 607) {
     // For older versions of the linker, use the legacy filelist method instead.
     ResponseSupport = {ResponseFileSupport::RF_FileList, llvm::sys::WEM_UTF8,
                        "-filelist"};
   }
 
-  const char *Exec = Args.MakeArgString(getToolChain().GetLinkerPath());
   std::unique_ptr<Command> Cmd = std::make_unique<Command>(
       JA, *this, ResponseSupport, Exec, CmdArgs, Inputs, Output);
   Cmd->setInputFileList(std::move(InputFileList));
   C.addCommand(std::move(Cmd));
 }
 
 void darwin::Lipo::ConstructJob(Compilation &C, const JobAction &JA,
                                 const InputInfo &Output,

clang/test/Driver/darwin-ld-demangle-lld.c

+// With -fuse-ld=lld, -demangle is always passed to the linker on Darwin.
+
+// RUN: %clang -### -fuse-ld=lld %s 2>&1 | FileCheck %s
+// FIXME: Remove ld.darwinnew once it's the default (and only) mach-o lld.
+// RUN: %clang -### -fuse-ld=lld.darwinnew %s 2>&1 | FileCheck %s
+// CHECK: -demangle

lld/Common/Strings.cpp

 #include <mutex>
 #include <vector>
 
 using namespace llvm;
 using namespace lld;
 
 // Returns the demangled C++ symbol name for name.
 std::string lld::demangleItanium(StringRef name) {
-  // itaniumDemangle can be used to demangle strings other than symbol
-  // names which do not necessarily start with "_Z". Name can be
-  // either a C or C++ symbol. Don't call demangle if the name
-  // does not look like a C++ symbol name to avoid getting unexpected
-  // result for a C symbol that happens to match a mangled type name.
-  if (!name.startswith("_Z"))
+  // itaniumDemangle() can be called for all symbols. Only demangle C++ symbols,
+  // to avoid getting unexpected result for a C symbol that happens to match a
+  // mangled type name such as "Pi" (which would demangle to "int*").
+  if (!name.startswith("_Z") && !name.startswith("__Z") &&
+      !name.startswith("___Z") && !name.startswith("____Z"))
     return std::string(name);
 
   return demangle(std::string(name));
 }
 
 SingleStringMatcher::SingleStringMatcher(StringRef Pattern) {
   if (Pattern.size() > 2 && Pattern.startswith("\"") &&
       Pattern.endswith("\"")) {

lld/MachO/Config.h

struct Configuration {
   bool staticLink = false;
   bool isPic = false;
   bool headerPadMaxInstallNames = false;
   bool searchDylibsFirst = false;
   bool saveTemps = false;
   uint32_t headerPad;
   llvm::StringRef installName;
   llvm::StringRef outputFile;
+  bool demangle = false;
   llvm::MachO::Architecture arch;
   PlatformInfo platform;
   llvm::MachO::HeaderFileType outputType;
   std::vector<llvm::StringRef> systemLibraryRoots;
   std::vector<llvm::StringRef> librarySearchPaths;
   std::vector<llvm::StringRef> frameworkSearchPaths;
   std::vector<llvm::StringRef> runtimePaths;
   llvm::DenseMap<llvm::StringRef, SymbolPriorityEntry> priorities;

lld/MachO/Driver.cpp

config->installName =
       args.getLastArgValue(OPT_install_name, config->outputFile);
   config->headerPad = args::getHex(args, OPT_headerpad, /*Default=*/32);
   config->headerPadMaxInstallNames =
       args.hasArg(OPT_headerpad_max_install_names);
   config->outputType = getOutputType(args);
   config->runtimePaths = args::getStrings(args, OPT_rpath);
   config->allLoad = args.hasArg(OPT_all_load);
   config->forceLoadObjC = args.hasArg(OPT_ObjC);
+  config->demangle = args.hasArg(OPT_demangle);
 
   if (const opt::Arg *arg = args.getLastArg(OPT_static, OPT_dynamic))
     config->staticLink = (arg->getOption().getID() == OPT_static);
 
   config->systemLibraryRoots = getSystemLibraryRoots(args);
   config->librarySearchPaths =
       getLibrarySearchPaths(args, config->systemLibraryRoots);
   config->frameworkSearchPaths =

lld/MachO/Options.td

 def no_data_in_code_info : Flag<["-"], "no_data_in_code_info">,
      HelpText<"This option is undocumented in ld64">,
      Flags<[HelpHidden]>,
      Group<grp_undocumented>;
 def debug_snapshot : Flag<["-"], "debug_snapshot">,
      HelpText<"This option is undocumented in ld64">,
      Flags<[HelpHidden]>,
      Group<grp_undocumented>;
 def demangle : Flag<["-"], "demangle">,
-     HelpText<"This option is undocumented in ld64">,
-     Flags<[HelpHidden]>,
-     Group<grp_undocumented>;
+     HelpText<"Demangle symbol names in diagnostics">;
 def dyld_env : Flag<["-"], "dyld_env">,
      HelpText<"This option is undocumented in ld64">,
      Flags<[HelpHidden]>,
      Group<grp_undocumented>;
 def encryptable : Flag<["-"], "encryptable">,
      HelpText<"This option is undocumented in ld64">,
      Flags<[HelpHidden]>,
      Group<grp_undocumented>;

lld/MachO/Symbols.cpp

uint64_t Defined::getFileOffset() const {
   }
   return isec->getFileOffset() + value;
 }
 
 void LazySymbol::fetchArchiveMember() { file->fetch(sym); }
 
 // Returns a symbol for an error message.
 std::string lld::toString(const Symbol &sym) {
-  if (Optional<std::string> s = demangleItanium(sym.getName()))
-    return *s;
+  if (config->demangle)
+    return demangleItanium(sym.getName());
   return std::string(sym.getName());
 }
 
 uint64_t DSOHandle::getVA() const { return header->addr; }
 
 uint64_t DSOHandle::getFileOffset() const { return header->fileOff; }
 
 constexpr StringRef DSOHandle::name;

lld/MachO/Writer.cpp

for (InputSection *isec : inputSections) {
     // it doesn't actually end up in the final binary. TODO: filtering it out
     // before Writer runs might be cleaner...
     if (isec->segname == segment_names::ld)
       continue;
 
     for (Reloc &r : isec->relocs) {
       if (auto *s = r.referent.dyn_cast<lld::macho::Symbol *>()) {
         if (isa<Undefined>(s))
-          error("undefined symbol " + s->getName() + ", referenced from " +
+          error("undefined symbol " + toString(*s) + ", referenced from " +
                 sys::path::filename(isec->file->getName()));
         else
           target->prepareSymbolRelocation(s, isec, r);
       } else {
         assert(r.referent.is<InputSection *>());
         if (!r.pcrel)
           in.rebase->addEntry(isec, r.offset);
       }

lld/test/ELF/undef.s

 
 # CHECK:      error: undefined symbol: vtable for Foo 
 # CHECK-NEXT: >>> referenced by undef.s
 # CHECK-NEXT: >>>               {{.*}}:(.text+0x15)
 # CHECK-NEXT: >>> the vtable symbol may be undefined because the class is missing its key function (see https://lld.llvm.org/missingkeyfunction)
 
 # Check that this symbol isn't demangled
 
-# CHECK:      error: undefined symbol: __Z3fooi
-# CHECK-NEXT: >>> referenced by undef.s
-# CHECK-NEXT: >>>               {{.*}}:(.text+0x1A)
-
 # CHECK:      error: undefined symbol: zed2
 # CHECK-NEXT: >>> referenced by {{.*}}.o:(.text+0x0) in archive {{.*}}2.a
 
 # CHECK:      error: undefined symbol: zed3
 # CHECK-NEXT: >>> referenced by undef-debug.s:3 (dir{{/|\\}}undef-debug.s:3)
 # CHECK-NEXT: >>>               {{.*}}.o:(.text+0x0)
 
 # CHECK:      error: undefined symbol: zed4

 .file "undef.s"
 
   .globl _start
 _start:
   call foo
   call bar
   call zed1
   call _Z3fooi
   call _ZTV3Foo
-  call __Z3fooi

lld/test/MachO/demangle.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
+
+# RUN: not %lld %t.o -o /dev/null 2>&1 | FileCheck %s
+# RUN: not %lld -demangle %t.o -o /dev/null 2>&1 | \
+# RUN:     FileCheck --check-prefix=DEMANGLE %s
+
+# CHECK: undefined symbol __Z1fv
+# DEMANGLE: undefined symbol f()
+
+.globl _main
+_main:
+  callq __Z1fv
+  ret

lld/test/MachO/silent-ignore.test

 RUN: %lld -v \
-RUN:   -demangle \
 RUN:   -dynamic \
 RUN:   -no_deduplicate \
 RUN:   -lto_library /lib/foo \
 RUN:   -macosx_version_min 0 \
 RUN:   -dependency_info /path/to/dependency_info.dat \
 RUN:   -mllvm -time-passes \
 RUN:   -objc_abi_version 2 \
 RUN:   -ios_simulator_version_min 9.0.0 \
 RUN:   -sdk_version 13.2
 RUN: not %lld -v --not-an-ignored-argument 2>&1 | FileCheck %s
 CHECK: error: unknown argument: --not-an-ignored-argument

lld/tools/lld/CMakeLists.txt

target_link_libraries(lld
   lldMinGW
   lldWasm
   )
 
 install(TARGETS lld
   RUNTIME DESTINATION bin)
 
 if(NOT LLD_SYMLINKS_TO_CREATE)
-  set(LLD_SYMLINKS_TO_CREATE lld-link ld.lld ld64.lld wasm-ld)
+  set(LLD_SYMLINKS_TO_CREATE
+      lld-link ld.lld ld64.lld ld64.darwinnew.lld wasm-ld)
 endif()
 
 foreach(link ${LLD_SYMLINKS_TO_CREATE})
   add_lld_symlink(${link} lld)
 endforeach()

llvm/utils/gn/secondary/lld/tools/lld/BUILD.gn

 import("//llvm/utils/gn/build/symlink_or_copy.gni")
 
 symlinks = [
   "lld-link",
   "ld.lld",
   "ld64.lld",
+  "ld64.darwinnew.lld",
   "wasm-ld",
 ]
 foreach(target, symlinks) {
   symlink_or_copy(target) {
     deps = [ ":lld" ]
     source = "lld"
     output = "$root_out_dir/bin/$target"
   }