This is an archive of the discontinued LLVM Phabricator instance.

Differential D110464

[lld-macho][nfc] Added some notes on deliberate differences btw LLD vs LD64
ClosedPublic

Authored by oontvoo on Sep 24 2021, 6:41 PM.

Details

Reviewers
int3
gkm
Group Reviewers
Restricted Project
Commits
rG6b715e9c4d9c: [lld-macho][nfc] Added some notes on deliberate differences btw LD64 vs LLD…
Summary

For future references and to help with debugging crashes, this could be useful.

Diff Detail

Event Timeline

oontvoo created this revision.Sep 24 2021, 6:41 PM
Herald added a reviewer: gkm. · View Herald TranscriptSep 24 2021, 6:41 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 24 2021, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2021, 6:41 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B125678: Diff 375005.Sep 24 2021, 6:53 PM
thevinster added a subscriber: thevinster.Sep 24 2021, 9:39 PM
thevinster added inline comments.
lld/MachO/notes.txt
1 ↗(On Diff #375005)

I presume these are differences in ld64 that we don't want to replicate in LLD? If so, I think it might be good to reflect something around those lines to not confuse the reader that this is some sort of todo list for LLD.

5 ↗(On Diff #375005)

It might seem more useful if we clarify what those criteria are?

6–7 ↗(On Diff #375005)
11 ↗(On Diff #375005)
15 ↗(On Diff #375005)

Are you planning to continue this section here?

18 ↗(On Diff #375005)

If we're planning to check this in, it might seem best to leave FIXME's out of notes. It's not code and you could always add this in when you remember. :)

oontvoo added a comment.Sep 27 2021, 10:43 AM

@thevinsterI presume these are differences in ld64 that we don't want to replicate in LLD? If so, I think it might be good to reflect something around those lines to not confuse the reader that this is some sort of todo list for LLD.

Right, this is not meant to be a bug-list or TODO list, so to speak, but rather where we say , here are the things we deliberately differ from LD64 (and why).
Some of the differences, you can point at a single piece of code, but some others are not so self-contained ... so I thought might be easier to put it here. (Also faster to look up than searching thru the code)

int3 requested changes to this revision.Oct 18 2021, 12:04 PM

We could just put in the paragraph about -ObjC for now. The symbol behavior is something that should definitely be included once we can make it more precise.

Can we make this a .rst file? Ultimately we should put this under lld/docs so we have nice web-readable documentation, and all the files there are .rst. But while we're drafting it, it can stay under MachO/.

lld/MachO/notes.txt
5 ↗(On Diff #375005)

it's partially alphabetical IIRC. but yeah, we should be more precise here

6–7 ↗(On Diff #375005)

"metadata" and "content" are kind of vague terms here. I suppose content is Defined::value...

This revision now requires changes to proceed.Oct 18 2021, 12:04 PM
oontvoo marked 7 inline comments as done.Oct 20 2021, 6:03 PM
oontvoo added inline comments.
lld/MachO/notes.txt
15 ↗(On Diff #375005)

I was undecided - (we're imitating Ld64 now but may revert that in a very near future ... let's leave this out for now)

18 ↗(On Diff #375005)

I was hoping someone would clarify this cos I'd forgotten what the subtlety was ... :)

oontvoo updated this revision to Diff 381123.Oct 20 2021, 6:04 PM
oontvoo marked an inline comment as done.

addressed review comments

Harbormaster completed remote builds in B129857: Diff 381123.Oct 20 2021, 6:17 PM
int3 accepted this revision.Oct 20 2021, 6:47 PM

Thanks!

lld/MachO/ld64-vs-lld.rst
13

It -> LD64 for clarity

This revision is now accepted and ready to land.Oct 20 2021, 6:47 PM
oontvoo updated this revision to Diff 381132.Oct 20 2021, 7:39 PM
oontvoo marked an inline comment as done.

addressed comment

oontvoo added a comment.Oct 20 2021, 7:40 PM

Thanks!

This revision was landed with ongoing or failed builds.Oct 20 2021, 7:42 PM
Closed by commit rG6b715e9c4d9c: [lld-macho][nfc] Added some notes on deliberate differences btw LD64 vs LLD… (authored by oontvoo). · Explain Why
This revision was automatically updated to reflect the committed changes.
oontvoo added a commit: rG6b715e9c4d9c: [lld-macho][nfc] Added some notes on deliberate differences btw LD64 vs LLD….
Harbormaster completed remote builds in B129863: Diff 381132.Oct 20 2021, 7:51 PM

Revision Contents

PathSize
lld/
MachO/
16 lines

Diff 381133

lld/MachO/ld64-vs-lld.rst

  • This file was added.
==================
LD64 vs LLD-MACHO
==================
This doc lists all significant deliberate differences in behavior between LD64 and LLD-MachO.
ObjC symbols treatment
**********************
There are differences in how LLD and LD64 handle ObjC symbols loaded from archives.
- LD64:
* Duplicate ObjC symbols from the same archives will not raise an error. LD64 will pick the first one.
* Duplicate ObjC symbols from different archives will raise a "duplicate symbol" error.
int3Unsubmitted
Done
ReplyInline Actions
- LD64:
- * Duplicate ObjC symbols from the same archives is not an error. It will pick the first one.
+ * Duplicate ObjC symbols from the same archive will not raise an error. LD64 will pick the first one.
* Duplicate ObjC symbols from different archives will raise a "duplicate symbol" error.

It -> LD64 for clarity

int3: It -> LD64 for clarity
- LLD:
* Duplicate symbols, regardless of which archives they are from, will raise errors.