michaeleisel / zld

A faster version of Apple's linker
MIT License
1.19k stars 50 forks source link

Transparent fallback on error to Apple ld #36

Closed michaeleisel closed 4 years ago

michaeleisel commented 4 years ago

Since zld failing at scale can be very bad for dev productivity, perhaps it would be good if, whenever zld errors out, it tries invoking Apple ld with the same arguments

rmaz commented 4 years ago

My concern with this would be no one would notice until the link time regressions started rolling in. Having it controllable with a flag should cover this though.

michaeleisel commented 4 years ago

Yeah, I think it'd be important to have a big "WARNING: zld failed, fell back to ld. This indicates an error with zld and will prevent its performance gains. Please file an issue.". We could also have a notification in the top-right corner

milend commented 4 years ago

@michaeleisel I guess you want to implement logic similar to if (zld_fails_where_ld_would_have_not) then run_ld().

I feel like that would be very hard to detect and as @rmaz notes, you can get a false sense of zld working just fine without any signal.

With regards to behavioural differences of zld vs ld64 (e.g., #37), a strategy might to detect cases where zld is known to be unsupported and forward to ld in those cases only.

michaeleisel commented 4 years ago

Why not just run ld to see if it would work or not? Performance? I'd imagine that most issues would cause ld to fail pretty quickly in its run.

michaeleisel commented 4 years ago

I think that we can be as in-your-face as we want with the use of notifications in the corner

milend commented 4 years ago

@michaeleisel:

Why not just run ld to see if it would work or not? Performance?

Either way, an option would be ideal and then the tradeoffs can be picked according to the deployment constraints.

michaeleisel commented 4 years ago

@milend yeah, an option can't hurt. How would it play it out for you in practice? When would you turn on this flag, and in what situations, such as arm64_32, would you want a fast-path to call into ld?

milend commented 4 years ago

@michaeleisel In practice, I now fallback to ld64 in case we're linking an ARM64_32 binary, otherwise we use zld.

The way I see it moving forwards is to keep a list of incompatible options and fallback if we detect such an option. For example, I'd expect Xcode 12 to use a modified linker, same as Xcode 11 did, where certain scenarios might not link correctly until Apple do another open source dump.

michaeleisel commented 4 years ago

Another mitigation I may use is to just print:

==========
Using non-standard linker. If linking fails, and you think it's spurious, do <steps to use Apple linker>
==========

and then do gating via some environment variable like NO_USE_ZLD

michaeleisel commented 4 years ago

(print this at the start of each invocation)

milend commented 4 years ago

Another mitigation I may use is to just print:

Do you plan to print when linking fails or on every run?

Two further aspects to think about:

and then do gating via some environment variable like NO_USE_ZLD

That would be useful. I'd also suggest supporting a parameter option that acts as an escape hatch as that one can easily be appended in specific config files depending on build system.

michaeleisel commented 4 years ago

My one concern, as I'm thinking about this, is that if we allow engineers that escape hatch, then CI can become the only thing checking that linker invocations are successful. The engineer may unknowingly try to check in code that causes non-deterministic failures in zld, without having had the chance to observe that

milend commented 4 years ago

Yup, agreed. I don't think we need any fallback apart from ARM64_32 since we know that's not supported. For everything else, I'd rather we get signal from engineers and trust that the linker works correctly. If we see more gaps/failures in the future, we can rethink.

At present, I have a custom zld wrapper that just detects ARM64_32 and calls into ld64 rather than zld and that works.

michaeleisel commented 4 years ago

Did you see my comment on the other issue? It may well be supported, unless you've tested it and seen otherwise

michaeleisel commented 4 years ago

closing for now, we can move the arm64_32 discussion to that gh issue