Page MenuHomePhabricator

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

Authored by oontvoo on Sep 30 2021, 9:57 PM.

Details

Reviewers
gkm
int3
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

oontvoo created this revision.Sep 30 2021, 9:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
oontvoo requested review of this revision.Sep 30 2021, 9:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 9:57 PM
xgupta added a subscriber: xgupta.Oct 1 2021, 12:16 AM

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.

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?

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 updated this revision to Diff 376579.Oct 1 2021, 11:13 AM
oontvoo marked an inline comment as done.

addressed comments

oontvoo marked an inline comment as done.Oct 1 2021, 11:14 AM
oontvoo added inline comments.
lld/MachO/DriverUtils.cpp
86 ↗(On Diff #376423)

ok, will make these optional params

oontvoo marked an inline comment as done.Oct 1 2021, 11:31 AM

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

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 planned changes to this revision.Oct 6 2021, 1:26 PM
oontvoo updated this revision to Diff 378214.Oct 8 2021, 7:58 AM

updated diff

oontvoo retitled this revision from [lld-macho] Check for errors when the response file doesn't exist. to [lld-macho] Downgrade missing -arch initially to error..Oct 8 2021, 7:58 AM
oontvoo edited the summary of this revision. (Show Details)
oontvoo edited the summary of this revision. (Show Details)
thevinster added inline comments.
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...

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

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 requested changes to this revision.Oct 18 2021, 12:06 PM
int3 added a subscriber: int3.

agree, earlier approach was nicer

This revision now requires changes to proceed.Oct 18 2021, 12:06 PM