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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added a comment.

In D110904#3053412 <https://reviews.llvm.org/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.



================
Comment at: lld/MachO/Driver.cpp:1347
+    // good.
+    if (target->isPlaceHolder())
+      fatal("unknown target platform - giving up");
----------------
thevinster wrote:
> 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...
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 ...)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110904/new/

https://reviews.llvm.org/D110904