[lld-macho] Implement option: -undefined TREATMENT

[gkm]

Summary

TREATMENT can be error, warning, suppress, or dynamic_lookup
The dymanic_lookup remains unimplemented for now.

[int3]

lld/MachO/Config.h

33–39

it doesn't look like we're actually using the numeric values, so we don't need to assign them. (: unsigned is likewise unnecessary)

lld/MachO/Driver.cpp

611–612

I think it would be nice set the treatment to ::error if the option argument is invalid. Then we won't need to handle the unknown case later on

lld/MachO/SymbolTable.cpp

158–159

can these be StringRefs?

176

with the change suggested above, this could be llvm_unreachable instead of fatal

lld/MachO/SymbolTable.h

58

I think this function is a verb-y function but the current name is noun-ish. How about naming it treatUndefinedSymbol? (ELF/COFF use reportUndefinedSymbol, but since we're not always going to report it...)

lld/MachO/Writer.cpp

415

doesn't seem like undefinedSymbol's return value is used anywhere. Can it just be a void function?

[gkm]

lld/MachO/SymbolTable.cpp

176

I dropped default: entirely, because clang warns that it is unused as every enum case is explicitly handled.

lld/MachO/SymbolTable.h

58

Since the API differs, I chose treatUndefinedSymbol()

lld/MachO/Writer.cpp

415

The bool was intended to accommodate use for the config->outputType == MH_EXECUTE case in lld::macho::link(), but I think it should always error() there. I made it void, since that intention was unrealized. We can always revise to return bool if/when we have a real use for it.

[gkm]

lld/MachO/SymbolTable.cpp

176

I thought I could drop unknown from the enum, but that wasn't workable for giving a diagnostic on StringSwitch<UndefinedSymbolTreatment>(...).Default(...), so default: llvm_unreachable() is necessary after all.

[gkm]

• respond to review feedback
• add test case

[int3]

lgtm

lld/MachO/SymbolTable.cpp

176

you could also have case UndefinedSymbolTreatment::unknown: instead of default: (while retaining the llvm_unreachable) -- that way we still benefit from Clang's exhaustiveness checks

177

s/unkown/unknown/

lld/test/MachO/invalid/undefined-symbol.s

13–14

fwiw it appears that the ELF and COFF ports use the following format:

error: undefined symbol: $SYM
>>> referenced by $FILE

but we can always tweak our format later on

[gkm]

• finalize according to review feedback

[int3]

lld/test/MachO/invalid/stub-link.s

11–12

this string should probably be changed too?

lld/test/MachO/treat-undef-sym.s

15

one final nit: why not include the filename here too? Or, if we're just interested in checking for error vs warning, then we could just leave out the check for the "referenced by" line. Checking that those two words exist in isolation doesn't seem super useful...

(thanks for changing the format string btw)

[smeenai]

lld/MachO/SymbolTable.cpp

160

Nit: using empty for this purpose is more idiomatic than size.

[gkm]

• Use .empty() rather than .size() != 0 for existence check

[gkm]

lld/test/MachO/invalid/stub-link.s

11–12

Ah, I missed this one because it is unsupported on Darwin. I will also test on Linux.

lld/test/MachO/treat-undef-sym.s

15

The intention is to verify that the >>> referenced by line occurs. The complete line would include the object filename %t.o, but %t is not available in that context.

[int3]

lld/test/MachO/treat-undef-sym.s

15

why not make %t available then via -DFILE?

[gkm]

• Fix "undefined symbol:" expected strings in more tests

[gkm]

• s/Twine/std::string/ for message in treatUndefinedSymbol()

[thakis]

As far as I can tell, ld64 only accepts -undefined warning and -undefined suppress with -flat_namespace (which lld/MachO doesn't yet implement). That makes sense since with a flat namespace the undefineds can be found at dynamic link time, but with two-level lookup they can't since they containing framework isn't known (…right?).

Is it intentional that we deviate from ld64 here?

% clang main.c -Wl,-undefined,suppress
ld: can't use -undefined warning or suppress with -twolevel_namespace
clang: error: linker command failed with exit code 1 (use -v to see invocation)
% clang main.c -Wl,-undefined,warning
ld: can't use -undefined warning or suppress with -twolevel_namespace
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[gkm]

@thakis, that's a bug. There is no reason to deviate from ld64.

lld/MachO/Config.h

 struct SymbolPriorityEntry;
 
 struct PlatformInfo {
   llvm::MachO::PlatformKind kind;
   llvm::VersionTuple minimum;
   llvm::VersionTuple sdk;
 };
 
+enum class UndefinedSymbolTreatment {
+  unknown,
+  error,
+  warning,
+  suppress,
+  dynamic_lookup,
+};
+
 struct Configuration {
   Symbol *entry;
   bool hasReexports = false;
   bool allLoad = false;
   bool forceLoadObjC = false;
   bool staticLink = false;
   bool implicitDylibs = false;
   bool isPic = false;
   bool headerPadMaxInstallNames = false;
   bool printEachFile = false;
   bool printWhyLoad = false;
   bool searchDylibsFirst = false;
   bool saveTemps = false;
   uint32_t headerPad;
   uint32_t dylibCompatibilityVersion = 0;
   uint32_t dylibCurrentVersion = 0;
   llvm::StringRef installName;
   llvm::StringRef outputFile;
   llvm::StringRef ltoObjPath;
   bool demangle = false;
   llvm::MachO::Architecture arch;
   PlatformInfo platform;
+  UndefinedSymbolTreatment undefinedSymbolTreatment =
+      UndefinedSymbolTreatment::error;
   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

static void handlePlatformVersion(const opt::Arg *arg) {
   // llvm::VersionTuple accepts no more than 4 components
   // Has Apple ever published version strings with 5 components?
   if (config->platform.minimum.tryParse(minVersionStr))
     error(Twine("malformed minimum version: ") + minVersionStr);
   if (config->platform.sdk.tryParse(sdkVersionStr))
     error(Twine("malformed sdk version: ") + sdkVersionStr);
 }
 
+static void handleUndefined(const opt::Arg *arg) {
+  StringRef treatmentStr = arg->getValue(0);
+  config->undefinedSymbolTreatment =
+      llvm::StringSwitch<UndefinedSymbolTreatment>(treatmentStr)
+          .Case("error", UndefinedSymbolTreatment::error)
+          .Case("warning", UndefinedSymbolTreatment::warning)
+          .Case("suppress", UndefinedSymbolTreatment::suppress)
+          .Case("dynamic_lookup", UndefinedSymbolTreatment::dynamic_lookup)
+          .Default(UndefinedSymbolTreatment::unknown);
+  if (config->undefinedSymbolTreatment == UndefinedSymbolTreatment::unknown) {
+    warn(Twine("unknown -undefined TREATMENT '") + treatmentStr +
+         "', defaulting to 'error'");
+    config->undefinedSymbolTreatment = UndefinedSymbolTreatment::error;
+  }
+}
+
 static void warnIfDeprecatedOption(const opt::Option &opt) {
   if (!opt.getGroup().isValid())
     return;
   if (opt.getGroup().getID() == OPT_grp_deprecated) {
     warn("Option `" + opt.getPrefixedName() + "' is deprecated in ld64:");
     warn(opt.getHelpText());
   }
 }
case OPT_weak_l:
       break;
     case OPT_framework:
     case OPT_weak_framework:
       addFramework(arg->getValue(), opt.getID() == OPT_weak_framework);
       break;
     case OPT_platform_version:
       handlePlatformVersion(arg);
       break;
+    case OPT_undefined:
+      handleUndefined(arg);
+      break;
     default:
       break;
     }
   }
 
   config->isPic = config->outputType == MH_DYLIB ||
                   config->outputType == MH_BUNDLE || isPie(args);
 

lld/MachO/Options.td

 def U : Separate<["-"], "U">,
      MetaVarName<"<symbol>">,
      HelpText<"Allow <symbol> to have no definition">,
      Flags<[HelpHidden]>,
      Group<grp_resolve>;
 def undefined : Separate<["-"], "undefined">,
      MetaVarName<"<treatment>">,
      HelpText<"Handle undefined symbols according to <treatment>: error, warning, suppress, or dynamic_lookup (default is error)">,
-     Flags<[HelpHidden]>,
      Group<grp_resolve>;
 def rpath : Separate<["-"], "rpath">,
      MetaVarName<"<path>">,
      HelpText<"Add <path> to dyld search list for dylibs with load path prefix `@rpath/'">,
      Group<grp_resolve>;
 def commons : Separate<["-"], "commons">,
      MetaVarName<"<treatment>">,
      HelpText<"Resolve tentative definitions in dylibs according to <treatment>: ignore_dylibs, use_dylibs, error (default is ignore_dylibs)">,

lld/MachO/SymbolTable.h

public:
   Symbol *find(StringRef name);
 
 private:
   std::pair<Symbol *, bool> insert(StringRef name);
   llvm::DenseMap<llvm::CachedHashStringRef, int> symMap;
   std::vector<Symbol *> symVector;
 };
 
+extern void treatUndefinedSymbol(StringRef symbolName, StringRef fileName);
+
 extern SymbolTable *symtab;
 
 } // namespace macho
 } // namespace lld
 
 #endif

lld/MachO/SymbolTable.cpp

 //===- SymbolTable.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolTable.h"
+#include "Config.h"
 #include "InputFiles.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::macho;
if (!wasInserted) {
     // error message.
     if (isa<Defined>(s))
       error("found defined symbol with illegal name " + DSOHandle::name);
   }
   replaceSymbol<DSOHandle>(s, header);
   return s;
 }
 
+void lld::macho::treatUndefinedSymbol(StringRef symbolName,
+                                      StringRef fileName) {
+  std::string message = ("undefined symbol: " + symbolName).str();
+  if (!fileName.empty())
+    message += ("\n>>> referenced by " + fileName).str();
+  switch (config->undefinedSymbolTreatment) {
+  case UndefinedSymbolTreatment::suppress:
+    break;
+  case UndefinedSymbolTreatment::error:
+    error(message);
+    break;
+  case UndefinedSymbolTreatment::warning:
+    warn(message);
+    break;
+  case UndefinedSymbolTreatment::dynamic_lookup:
+    error("dynamic_lookup unimplemented for " + message);
+    break;
+  case UndefinedSymbolTreatment::unknown:
+    llvm_unreachable("unknown -undefined TREATMENT");
+  }
+}
+
 SymbolTable *macho::symtab;

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 " + toString(*s) + ", referenced from " +
-                toString(isec->file));
+          treatUndefinedSymbol(toString(*s), toString(isec->file));
         else
           target->prepareSymbolRelocation(s, isec, r);
       } else {
         assert(r.referent.is<InputSection *>());
         if (!r.pcrel)
           in.rebase->addEntry(isec, r.offset);
       }
     }

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()
+# CHECK: undefined symbol: __Z1fv
+# DEMANGLE: undefined symbol: f()
 
 .globl _main
 _main:
   callq __Z1fv
   ret

lld/test/MachO/invalid/stub-link.s

 ## FIXME: This test seems to be failing on some Google Mac buildbots for
 ## unclear reasons, so it's disabled for now. See D85404 for details.
 # UNSUPPORTED: darwin
 # REQUIRES: x86
 
 # RUN: mkdir -p %t
 #
 # RUN: llvm-mc -filetype obj -triple x86_64-apple-ios %s -o %t/test.o
 # RUN: not lld -flavor darwinnew -o %t/test -syslibroot %S/../Inputs/iPhoneSimulator.sdk -lSystem %t/test.o 2>&1 | FileCheck %s
 
-# CHECK-DAG: error: undefined symbol __cache_handle_memory_pressure_event
-# CHECK-DAG: error: undefined symbol _from_non_reexported_tapi_dylib
+# CHECK-DAG: error: undefined symbol: __cache_handle_memory_pressure_event
+# CHECK-DAG: error: undefined symbol: _from_non_reexported_tapi_dylib
 
 .section __TEXT,__text
 .global _main
 
 _main:
   movq __cache_handle_memory_pressure_event@GOTPCREL(%rip), %rax
   movq _from_non_reexported_tapi_dylib@GOTPCREL(%rip), %rax
   ret

lld/test/MachO/invalid/undefined-symbol.s

 # REQUIRES: x86
 # RUN: rm -rf %t
 # RUN: split-file %s %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/main.s -o %t/main.o
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
 # RUN: llvm-ar crs %t/foo.a %t/foo.o
 # RUN: not %lld -o /dev/null %t/main.o 2>&1 | \
 # RUN:     FileCheck %s -DSYM=_foo -DFILENAME=%t/main.o
 # RUN: not %lld -o /dev/null %t/main.o %t/foo.a 2>&1 | \
 # RUN:     FileCheck %s -DSYM=_bar -DFILENAME='foo.a(foo.o)'
 # RUN: not %lld -o /dev/null %t/main.o -force_load %t/foo.a 2>&1 | \
 # RUN:     FileCheck %s -DSYM=_bar -DFILENAME='foo.a(foo.o)'
-# CHECK: error: undefined symbol [[SYM]], referenced from [[FILENAME]]
+# CHECK: error: undefined symbol: [[SYM]]
+# CHECK-NEXT: >>> referenced by [[FILENAME]]
 
 #--- foo.s
 .globl _foo
 .text
 _foo:
   callq _bar
   retq
 
 #--- main.s
 .globl _main
 .text
 _main:
   callq _foo
   movq $0, %rax
   retq

lld/test/MachO/treat-undef-sym.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos %s -o %t.o
+# RUN: not %lld -undefined bogus -o /dev/null %t.o 2>&1 | \
+# RUN:     FileCheck %s -check-prefix=UNKNOWN
+# RUN: not %lld -undefined error -o /dev/null %t.o 2>&1 | \
+# RUN:     FileCheck %s -check-prefix=ERROR
+# RUN:     %no_fatal_warnings_lld -undefined warning -o /dev/null %t.o 2>&1 | \
+# RUN:     FileCheck %s -check-prefix=WARNING
+# RUN:     %lld -undefined suppress -o /dev/null %t.o 2>&1 | \
+# RUN:     FileCheck %s -check-prefix=SUPPRESS --allow-empty
+
+# ERROR: error: undefined symbol: _bar
+# ERROR-NEXT: >>> referenced by
+
+# WARNING: warning: undefined symbol: _bar
+# WARNING-NEXT: >>> referenced by
+
+# SUPPRESS-NOT: undefined symbol: _bar
+
+# UNKNOWN: unknown -undefined TREATMENT 'bogus'
+# UNKNOWN-NEXT: error: undefined symbol: _bar
+# UNKNOWN-NEXT: >>> referenced by
+
+.globl _main
+_main:
+  callq _bar
+  ret

lld/test/MachO/weak-reference.s

 # STRONG-YAML-LABEL: LazyBindOpcodes:
 # STRONG-YAML:        - Opcode:          BIND_OPCODE_SET_SYMBOL_TRAILING_FLAGS_IMM
 # STRONG-YAML-NEXT:     Imm:             0
 # STRONG-YAML-NEXT:     Symbol:          _foo_fn
 
 ## Weak references must still be satisfied at link time.
 # RUN: not %lld -lSystem %t/invalid.o -o /dev/null 2>&1 | FileCheck %s \
 # RUN:   --check-prefix=INVALID -DDIR=%t
-# INVALID: error: undefined symbol _missing, referenced from [[DIR]]/invalid.o
+# INVALID: error: undefined symbol: _missing
 
 #--- libfoo.s
 .globl _foo, _foo_fn, _weak_foo, _weak_foo_fn
 .weak_definition _weak_foo, _weak_foo_fn
 _foo:
 _foo_fn:
 _weak_foo:
 _weak_foo_fn: