[lld-macho] Downgrade missing -arch initially to error.

[oontvoo]

Summary

Two reasons:
- we could be able to infer the arch from the inputs (not implemented yet, tho) => it's a bit immature to die early when -arch is not specified
- and if a missing response file is passed in, the fatal() error on missing -arch prevented lld from diagnosing this. So this is also a bit confusing. (LD64 doesn't have this problem)

This patch delay the fatal() until after parsing input files.

[thakis]

Why do we need this? We currently print ld64.lld: error: cannot open @fooasdfdsaf.txt: No such file or directory which seems fine?

None of the other lld ports do this.

[thakis]

Oh, I see that function is new. Probably should do this for all ports in this CL then if you think this is worth doing.

lld/MachO/DriverUtils.cpp

86 ↗

(On Diff #376423)

I see this is a new function. Since this will be the only caller, can we add only the arguments we need, so that we don't have to pass all these default args?

91 ↗

(On Diff #376423)

Can we use checkError() instead?

[oontvoo]

In D110904#3036354, @thakis wrote:

We currently print ld64.lld: error: cannot open @fooasdfdsaf.txt: No such file or directory which seems fine?

Hmm which version printed this?
Right now all i'm getting was ("must specify -arch") for the macho port, which took me awhile to realise I had a typo in my response file name ..

bin/ld64.lld @foobar.txt
ld64.lld: error: must specify -arch

None of the other lld ports do this.

ELF seems to have it:

$ bin/ld.lld @foobar.txt
ld.lld: error: cannot open @foobar.txt: No such file or directory

[oontvoo]

addressed comments

[oontvoo]

lld/MachO/DriverUtils.cpp

86 ↗

(On Diff #376423)

ok, will make these optional params

[thakis]

Hmm which version printed this?

Trunk. (I did pass a -arch flag though.)

ELF seems to have it:

Can you check how it does it? It can't call that function that doesn't yet exist :)

(I guess we error out due to missing -arch before trying to open "@foo" as input file in MachO, which is why the -arch error is printed before the rsp error. Almost all users will use lld through the clang driver though, so in practice -arch will always be present.)

[oontvoo]

In D110904#3040045, @thakis wrote:

Hmm which version printed this?

Trunk. (I did pass a -arch flag though.)

ELF seems to have it:

Can you check how it does it? It can't call that function that doesn't yet exist :)

(I guess we error out due to missing -arch before trying to open "@foo" as input file in MachO, which is why the -arch error is printed before the rsp error. Almost all users will use lld through the clang driver though, so in practice -arch will always be present.)

Oh, I see what you're saying. Right, ELF doesn't explicitly check for the response file not found, either. Instead it'd just try to load it as an input file later if it's previously fail to expand the @foo.txt .

IMO, that's a bit confusing - the cl::ExpandResponseFiles() today already returns a bool flag to indicate whether it succeeded - callers should probably check it. (Neither macho nor elf does that today). My other patch only expand the ExpandResponseFiles only expands the function to include more error info ...

Happy to apply the same change here over to elf (probably in a separate patch tho).

[oontvoo]

rebase

[oontvoo]

updated diff

[thevinster]

lld/MachO/Driver.cpp

1347

I'm not sure how I feel about having all this code about a dummy struct just so we can avoid the typo scenario you mentioned. To me, it feels like an indirection just to get to the actual root cause. What's the difference between this and moving createTargetInfo here instead? It seems like the same result can be achieved and it's easier to reason about.

I'm curious to know what others think tho...

[thakis]

To be honest, I'm not sure this is really worth it. Almost everyone will invoke lld through the compiler driver.

But if you do want to have a nicer diag for missing rsp files, I like your previous approach of emitting a diag there better (assuming we add it to all drivers, and add the default args so the driver changes are small).

[oontvoo]

In D110904#3053412, @thakis wrote:

But if you do want to have a nicer diag for missing rsp files, I like your previous approach of emitting a diag there better (assuming we add it to all drivers, and add the default args so the driver changes are small).

The assumption in the previous patch was that if the util failed to read a foo from @foo as a file then, then it's an error - that's kind of wrong because it's too early for it to know - foo could have meant to be some other parameter (eg., @ loader_path)

Almost everyone will invoke lld through the compiler driver.

this may be true - but in general i still think it's worth it to diagnose errors correctly rather than giving some misleading ones (esp. as stuff gets more complicated). Also, there's still this unimplemented feature of inferring from input files - so perhaps it really shouldn't complain about unknown arch until *after* inputs have been parsed.

lld/MachO/Driver.cpp

1347

we can't move it because other steps needed at least some reference to the target (caveat: without overhauling a few things, which I could try ...)

[int3]

agree, earlier approach was nicer

lld/MachO/Arch/PlaceHolderTarget.cpp

+#include "InputFiles.h"
+#include "Symbols.h"
+#include "SyntheticSections.h"
+#include "Target.h"
+
+#include "lld/Common/ErrorHandler.h"
+#include "llvm/BinaryFormat/MachO.h"
+
+using namespace llvm;
+using namespace llvm::MachO;
+using namespace llvm::support::endian;
+using namespace lld;
+using namespace lld::macho;
+
+namespace {
+struct PlaceHolderTarget : TargetInfo {
+  PlaceHolderTarget() : TargetInfo(LP64()) {}
+
+  int64_t getEmbeddedAddend(MemoryBufferRef, uint64_t offset,
+                            const relocation_info) const override {
+    llvm_unreachable("not implemented");
+  }
+  void relocateOne(uint8_t *loc, const Reloc &, uint64_t va,
+                   uint64_t relocVA) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  void writeStub(uint8_t *buf, const Symbol &) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  void writeStubHelperHeader(uint8_t *buf) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  void writeStubHelperEntry(uint8_t *buf, const DylibSymbol &,
+                            uint64_t entryAddr) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  void relaxGotLoad(uint8_t *loc, uint8_t type) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  const RelocAttrs &getRelocAttrs(uint8_t type) const override {
+    llvm_unreachable("not implemented");
+  }
+
+  uint64_t getPageSize() const override { llvm_unreachable("not implemented"); }
+
+  bool isPlaceHolder() const override { return true; }
+};
+
+} // namespace
+namespace lld {
+namespace macho {
+
+TargetInfo *createPlaceHolderTargetInfo() {
+  static PlaceHolderTarget t;
+  return &t;
+}
+
+} // namespace macho
+} // namespace lld

lld/MachO/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS Options.td)
 tablegen(LLVM Options.inc -gen-opt-parser-defs)
 add_public_tablegen_target(MachOOptionsTableGen)
 
 include_directories(${LLVM_MAIN_SRC_DIR}/../libunwind/include)
 
 add_lld_library(lldMachO
   Arch/ARM.cpp
   Arch/ARM64.cpp
   Arch/ARM64Common.cpp
   Arch/ARM64_32.cpp
   Arch/X86_64.cpp
+  Arch/PlaceHolderTarget.cpp
   ConcatOutputSection.cpp
   Driver.cpp
   DriverUtils.cpp
   Dwarf.cpp
   ExportTrie.cpp
   ICF.cpp
   InputFiles.cpp
   InputSection.cpp

lld/MachO/Driver.cpp

auto add = [&ltoPolicy](Twine val) {
     if (!ltoPolicy.empty())
       ltoPolicy += ":";
     val.toVector(ltoPolicy);
   };
   for (const Arg *arg :
        args.filtered(OPT_thinlto_cache_policy, OPT_prune_interval_lto,
                      OPT_prune_after_lto, OPT_max_relative_cache_size_lto)) {
     switch (arg->getOption().getID()) {
-    case OPT_thinlto_cache_policy: add(arg->getValue()); break;
+    case OPT_thinlto_cache_policy:
+      add(arg->getValue());
+      break;
     case OPT_prune_interval_lto:
       if (!strcmp("-1", arg->getValue()))
         add("prune_interval=87600h"); // 10 years
       else
         add(Twine("prune_interval=") + arg->getValue() + "s");
       break;
     case OPT_prune_after_lto:
       add(Twine("prune_after=") + arg->getValue() + "s");
static PlatformKind parsePlatformVersion(const ArgList &args) {
   if (config->platformInfo.sdk.tryParse(sdkVersionStr))
     error(Twine("malformed sdk version: ") + sdkVersionStr);
   return platform;
 }
 
 // Has the side-effect of setting Config::target.
 static TargetInfo *createTargetInfo(InputArgList &args) {
   StringRef archName = args.getLastArgValue(OPT_arch);
-  if (archName.empty())
-    fatal("must specify -arch");
+  if (archName.empty()) {
+    error("must specify -arch");
+  }
   PlatformKind platform = parsePlatformVersion(args);
 
   config->platformInfo.target =
       MachO::Target(getArchitectureFromName(archName), platform);
 
   uint32_t cpuType;
   uint32_t cpuSubtype;
   std::tie(cpuType, cpuSubtype) = getCPUTypeFromArchitecture(config->arch());
 
   switch (cpuType) {
   case CPU_TYPE_X86_64:
     return createX86_64TargetInfo();
   case CPU_TYPE_ARM64:
     return createARM64TargetInfo();
   case CPU_TYPE_ARM64_32:
     return createARM64_32TargetInfo();
   case CPU_TYPE_ARM:
     return createARMTargetInfo(cpuSubtype);
   default:
-    fatal("missing or unsupported -arch " + archName);
+    // Empty arch would have been reported already.
+    if (!archName.empty())
+      error("unsupported -arch " + archName);
+    return createPlaceHolderTargetInfo();
   }
 }
 
 static UndefinedSymbolTreatment
 getUndefinedSymbolTreatment(const ArgList &args) {
   StringRef treatmentStr = args.getLastArgValue(OPT_undefined);
   auto treatment =
       StringSwitch<UndefinedSymbolTreatment>(treatmentStr)
#endif
   if (config->timeTraceEnabled)
     timeTraceProfilerInitialize(config->timeTraceGranularity, config->progName);
 
   {
     TimeTraceScope timeScope("ExecuteLinker");
 
     initLLVM(); // must be run before any call to addFile()
     createFiles(args);
+    // Now that files have been parsed, check again to make sure the target is
+    // good.
+    if (target->isPlaceHolder())
+      fatal("unknown target platform - giving up");
 
     config->isPic = config->outputType == MH_DYLIB ||
                     config->outputType == MH_BUNDLE ||
                     (config->outputType == MH_EXECUTE &&
                      args.hasFlag(OPT_pie, OPT_no_pie, true));
 
     // Now that all dylibs have been loaded, search for those that should be
     // re-exported.

lld/MachO/Target.h

public:
   }
 
   bool hasAttr(uint8_t type, RelocAttrBits bit) const {
     return getRelocAttrs(type).hasAttr(bit);
   }
 
   bool usesThunks() const { return thunkSize > 0; }
 
+  virtual bool isPlaceHolder() const { return false; }
+
   uint32_t magic;
   llvm::MachO::CPUType cpuType;
   uint32_t cpuSubtype;
 
   uint64_t pageZeroSize;
   size_t headerSize;
   size_t stubSize;
   size_t stubHelperHeaderSize;
public:
   // tombstone value for DenseMap<> and caused weird assertions for me.
   static constexpr uint64_t outOfRangeVA = 0xfull << 60;
 };
 
 TargetInfo *createX86_64TargetInfo();
 TargetInfo *createARM64TargetInfo();
 TargetInfo *createARM64_32TargetInfo();
 TargetInfo *createARMTargetInfo(uint32_t cpuSubtype);
+TargetInfo *createPlaceHolderTargetInfo();
 
 struct LP64 {
   using mach_header = llvm::MachO::mach_header_64;
   using nlist = structs::nlist_64;
   using segment_command = llvm::MachO::segment_command_64;
   using section = llvm::MachO::section_64;
   using encryption_info_command = llvm::MachO::encryption_info_command_64;
 

lld/test/MachO/reproduce.s

 # RUN: tar xf repro2.tar
 # RUN: cmp %t.dir/build2/foo.o repro2/%:t.dir/build2/foo.o
 # RUN: FileCheck %s --check-prefix=RSP2 < repro2/response.txt
 # RSP2-NOT:  {{^}}repro2{{[/\\]}}
 # RSP2:      {{[/\\]}}foo.o
 
 # RUN: cd repro2; ld64.lld @response.txt
 
+## Error case
+# RUN: rm -rf @no_such_file.txt        
+# RUN: not %lld @no_such_file.txt 2>&1 | FileCheck %s --check-prefix=NOFILE
+# NOFILE: error: cannot open @no_such_file.txt: No such file or directory
+        
 .globl _main
 _main:
   ret