Overall, lgtm. Minor comments

Would love to get some feedback whether there's interest in this or if there's a better way to solve the build speed issue.

I wonder whether you can consider binary patching the broken shared objects. I have done similar things for my employer (for symbol versioning, not for this STB_LOCAL instance) as well..
If binary patching doesn't look great, use --noinhibit-exec.

@MaskRay This is effectively a revert of https://github.com/llvm/llvm-project/commit/3ac94280245415be66cb1b603367c5f4f6d498e7 (but keeping the message intact). Do you have any suggestions how to get around this specific issue?

In D150464#4444716, @thevinster wrote:

@labrinea This change hits a crash when trying to build lld with thinLTO (i.e. stage2 lld) and with assertions enabled. I verified that the revision before this change works. I get the following assertion failure

opt: <workspace>/llvm-project/llvm/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From *) [To = llvm::ConstantInt, From = llvm::Constant]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

I reduced the issue and this can be reproed by building opt on this revision and then running

$ opt reduced.ll -o /dev/null -O3

where reduced.ll is https://reviews.llvm.org/P8311

Could you please take a look and advise the path forward?

cc: @smeenai

@labrinea This change hits a crash when trying to build lld with thinLTO (i.e. stage2 lld) and with assertions enabled. I verified that the revision before this change works. I get the following assertion failure

opt: <workspace>/llvm-project/llvm/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From *) [To = llvm::ConstantInt, From = llvm::Constant]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

Remove thumb field. Assert N_ARM_THUMB_DEF flag not set.

We've just recently updated our version of LLVM (I know we're slow...), and this patch breaks our builds when building LLVM for macOS Universal and with LTO. Specifically, this is the error I get when building for such configuration.

This might've been an oversight on my end. I think the choice to use the host linker to build for a target was to retain the previous behavior as close as possible while allowing the ability to override. Your situation seems to contradict this scenario unless you override specifically on the command line, which I can agree might not be desirable. Feel free to remove the default as long as existing builds all pass.

LGTM. Not sure why buildbots are complaining, but they don't look super related.

After some internal discussions, I'm going to abandon this in favor of a local internal patch since the current behavior is easier to reason. If other folks run into this issue and also want this behavior, I'm happy to bring this back.

We do handle systemPaths as optional right now in L186-L194. The issue is that the link arguments contain an explicit -L/usr/local/lib that doesn't exist.

Re-reading that, it seems to be the same issue (though our case is because of swift autolinking instead of symbols defined in the same archive). I think we should re-open that PR and track it for future references. Seeing that it could be more widespread, we might want to prioritize it.

I'm happy to commit to the idea that hashing on the contents is the wrong implementation. But, do people think this specific behavior is one that should still be fixed within LLD (with another implementation) or be fixed directly in the codebase (assuming this is possible)?

In D140592#4018461, @thakis wrote:

Imho it'd be nicer to add this to the "list of places where lld is different from ld64". From what I can tell, this isn't a _huge_ problem in practice (we haven't needed it until now), and there's some value in both sensible semantics and in having behavior that's consistent with the other llds too.

In D137368#3916144, @thakis wrote:

In D137368#3915933, @int3 wrote:

Does this allow us to undo D115416?

Possibly, I'll have to do some benchmarks. However I think it is likely still a performance improvement to do it in parallel

Sure, but having simpler code makes it easier to parallelize other parts. Doing complicated parallelism too early makes it harder to do parallelize things that would otherwise be easier to parallelize and might get us stuck in a local optimum.

(This is admittedly very hand-wavy!)

Rename dupSymReporter to dupSymDiags

Fixed up nits and naming

Update comment

Add flag to gate dead stripped duplicate symbols

Do you have data how much harder these are to fix than (say) the text deduping failures?

FWIW, I don't agree with this argument, and our doc listing intentional differences doesn't either. (Not deduping strings by default is a big difference and we're stricter in various other areas too.)

Overall, lgtm. Thanks!

Some minor comments. Overall, LGTM, I'll leave it to people more familiar with the code to stamp.

@int3 Thanks for the clarification! In that case, this change LGTM.

we would actually produce an invalid __eh_frame section in that case because the CIE associated with the unnecessary FDE would still get dead-stripped and we'd end up with a dangling FDE

Whoops, this might've been a copy paste error on my end. Thanks for catching it!

Could we run a benchmark on this just to see if it has any impact on link speed? I remember bind opcodes optimization had a slight link speed regression which was then put behind an -O2 flag since it isn't worth it for debug builds. I'm wondering if we need to do the same for the rebase opcodes.

LGTM

LGTM

Fixed up comments and renaming

Overall, I gave it some thought on the naming. To be consistent with a part of the naming (e.g. no_fatal_warnings_), it makes sense to have no-arg be as the prefix as well. We do have a mix of - and _ (e.g. lld-watchos and no_fatal_warnings_lld). I broke the tie and slightly prefer -. Hence, the result no-arg-lld.

Rename to no-arg-lld

Switch name to lld-noarg

why doesn't re-using %lld plus additional args, which would override the defalut ones provided by %lld anyways, work?

Unfortunately, we can't start with %lld anymore otherwise it would get replaced with the value of %lld and then just append -noarg which makes the command invalid. I'm happy to change it to %no-arg-lld.

Bikeshedding: How about %lld-noarg? It's a bit more descriptive and only two characters longer.

LGTM