This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Skip re-loading archive if already loaded
AbandonedPublic

Authored by thevinster on Dec 22 2022, 4:43 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

When an archive is loaded via an LC_LINKER_OPTION, loading it again will
cause duplicate symbols. This check has been added here. However,
this didn't handle the case where the archive paths are different. For example,
we could be loading a framework and that same framework can be bundled
in another static library. If we are force loading those objects via ObjC, we will
end up with duplicate symbols.

Here, add another check that hashes on the contents and skips loading an
archive's children if the module has already been loaded. This requires making
seen a global property instead of a class property so it can properly track the
different archives being loaded. This also matches ld64's behavior.

While I'm here, make the output test executables be unique so it's easier to debug.

Diff Detail

Event Timeline

thevinster created this revision.Dec 22 2022, 4:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 22 2022, 4:43 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
thevinster edited the summary of this revision. (Show Details)Dec 22 2022, 5:00 PM
thevinster edited the summary of this revision. (Show Details)
thevinster edited the summary of this revision. (Show Details)
thevinster published this revision for review.Dec 22 2022, 5:02 PM
thevinster edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2022, 5:02 PM
thakis added a subscriber: thakis.Dec 22 2022, 6:45 PM
thakis added inline comments.
lld/MachO/Driver.cpp
327
lld/MachO/InputFiles.cpp
2110

We're doing this based on buffer contents? Where in ld64's source does it do that? That seems like very surprising semantics (and kinda bad for perf).

thevinster added inline comments.Dec 27 2022, 12:22 AM
lld/MachO/InputFiles.cpp
2110

I don't know if there will be any significant perf regressions. I'll double check that on my end first before saying anything else. From what I can tell, it looks like ld64 hashes based on the contents of the Entry class. I agree that these are surprising semantics (especially the behavior of force loading two of the same object files from an archive), but this strange behavior is also implicitly depended upon by some of our builds. Whether or not this is the correct implementation, I'm merely trying to follow ld64's behavior. As far as implementation is concerned, this is the path of least resistance that'll solve our current duplicate symbol issue. I'm open to suggestions on how else this can be done (I've tried approaches where I only load the symbol if it hasn't been loaded but that ran into many edge cases).

thevinster added inline comments.Dec 27 2022, 4:58 PM
lld/MachO/InputFiles.cpp
2110

Benchmarking chromium framework,

           base           diff           difference (95% CI)
sys_time   2.050 ± 0.028  2.037 ± 0.027  [  -1.2% ..   -0.0%]
user_time  4.550 ± 0.040  4.589 ± 0.040  [  +0.5% ..   +1.2%]
wall_time  7.054 ± 0.067  7.084 ± 0.067  [  +0.0% ..   +0.8%]
samples    45             42

This is a very slight regression, and personally, I think it's worth the hit if it means us being even more compatible with ld64's behaviors.

int3 added a subscriber: int3.Dec 27 2022, 6:32 PM
int3 added inline comments.
lld/MachO/InputFiles.cpp
2110

Do you have a code pointer to where the Entry class is getting hashed in ld64?

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.

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.

I hear you on the sensible semantics, but I disagree that we need to have behavior that's consistent with the other lld ports. From what I can see, there are many things that the Mach-O port has introduced that differs from ELF and COFF ports (e.g. string alignment, async map writing, etc.) that at this point, I would even consider all three linkers to be distinct. Plus, it deviates from the goal of lld being a "drop-in" replacement. While I don't have strong feelings of whether this specific behavior needs to be implemented in lld, I'm curious to know where you draw the line when a behavior should deviate from ld64. In regards to whether this is a huge problem in practice, I don't think any of us here can make great arguments since everyone focuses on making their builds work. In your case, maybe this isn't a "huge problem", but in our case, it is.

lld/MachO/InputFiles.cpp
2110

If I'm understanding correctly, I believe this method is where the check happens whether an archive member has already been loaded. In particular, _instantiatedEntries (https://github.com/apple-oss-distributions/ld64/blob/main/src/ld/parsers/archive_file.cpp#L121) is a map of an Entry to the MemberState.

int3 added a comment.Dec 28 2022, 6:21 PM

So I'm kinda with @thakis on this. I would prefer we avoid hashing contents (assuming this is what ld64 is actually doing...)

I'm curious to know where you draw the line when a behavior should deviate from ld64

@thakis would probably say that he doesn't like the async map file stuff either ;)

IMO the async map file stuff is different because

  1. it's a strict implementation detail -- there's no observable change aside from perf
  2. though the perf impact of hashing archives in the chromium_framework build is small, this seems like the kind of thing that could surprise us down the line. hashing entire files is in general Not Cheap, and it seems quite possible that we'll one day encounter a large archive member that causes perf issues. We *could* try to hack around it by introducing a flag such as --no-hash-archive-member=, but that's super ugly

Or in other words, the async map file has an ugly implementation, but the final behavior is easy enough to reason about / build on top of. Hashing archive contents has a relatively simple implementation, but the resulting behavior feels off...

lld/MachO/InputFiles.cpp
2110

_instantiatedEntries is a vanilla std::map w/o a custom comparator (https://github.com/apple-oss-distributions/ld64/blob/main/src/ld/parsers/archive_file.cpp#L121), so I don't think it's hashing the contents of Entry, just the pointer value itself

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

int3 added a comment.Dec 29 2022, 12:17 PM

So I played around with the test a bit, creating a has-objc-symbol-2.s file that is identical except with a section named has_objc_symbol2 instead of has_objc_symbol. There is no dup symbol error either, demonstrating that ld64 isn't hashing contents (at least not the full contents).

Anyway that investigation jogged my memory -- I think this the same issue as https://github.com/llvm/llvm-project/issues/50817#issuecomment-981045478. As that comment indicates, I'd prefer if we fixed the issue in the builds, if that's possible.

We should also document that behavioral difference so we don't forget about it again heh

thevinster abandoned this revision.Dec 29 2022, 6:11 PM

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.