This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Implement LC_UUID
ClosedPublic

Authored by int3 on Oct 14 2020, 12:24 PM.

Details

Reviewers
compnerd
smeenai
Group Reviewers
Restricted Project
Commits
rGb86908171ea8: [lld-macho] Implement LC_UUID
Summary

Apple devtools use this to locate the dSYM files for a given
binary.

The UUID is computed based on an MD5 hash of the binary's contents. In order to
hash the contents, we must first write them, but LC_UUID itself must be part of
the written contents in order for all the offsets to be calculated correctly.
We resolve this circular paradox by first writing an LC_UUID with an all-zero
UUID, then updating the UUID with its real value later.

I'm not sure there's a good way to test that the value of the UUID is
"as expected", so I've just checked that it's present.

Diff Detail

Event Timeline

int3 created this revision.Oct 14 2020, 12:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 12:24 PM
int3 requested review of this revision.Oct 14 2020, 12:24 PM
compnerd accepted this revision.Oct 15 2020, 8:45 AM
This revision is now accepted and ready to land.Oct 15 2020, 8:45 AM
ayrivera removed a subscriber: ayrivera.

Hi,

Sorry to post this around a week late. I've been noticing that the same lit test that failed here (windows > lld.ELF/invalid::symtab-sh-info.s) is failing in my local build. I tried to debug it and noticed that a dense map is being deallocated, but it is a nullpointer. Is this a known issue? I apologies if I'm asking in the wrong place. Thanks for your time.

int3 added a comment.Oct 25 2020, 8:53 AM

The failing test is unrelated to the changes made in this diff (also this diff hasn't been landed yet)

The failing test is unrelated to the changes made in this diff (also this diff hasn't been landed yet)

Hi,

Yes, I understand that the failure isn't related to your changes. I've seen the same failure in other changes. I'm just asking if this failure has been discussed. Would it be best mailing to llvm-dev? Sorry for the naïve question. Thanks for your time and help.

int3 added a comment.Oct 26 2020, 3:02 PM

Yeah I think llvm-dev would probably be a better place to find answers

Hi,

Sorry to post this around a week late. I've been noticing that the same lit test that failed here (windows > lld.ELF/invalid::symtab-sh-info.s) is failing in my local build. I tried to debug it and noticed that a dense map is being deallocated, but it is a nullpointer. Is this a known issue? I apologies if I'm asking in the wrong place. Thanks for your time.

D88348 looks like it has the fix for this.

Hi,

Sorry to post this around a week late. I've been noticing that the same lit test that failed here (windows > lld.ELF/invalid::symtab-sh-info.s) is failing in my local build. I tried to debug it and noticed that a dense map is being deallocated, but it is a nullpointer. Is this a known issue? I apologies if I'm asking in the wrong place. Thanks for your time.

D88348 looks like it has the fix for this.

Thank you!

smeenai accepted this revision.Nov 9 2020, 6:50 PM

LGTM

This revision was landed with ongoing or failed builds.Nov 10 2020, 12:20 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 16 2020, 3:52 PM

Any reason this uses md5? In the COFF and ELF ports, we found that using md5 as debug info uuid is a bottleneck, but xxhash is fine. https://reviews.llvm.org/D51956 has some discussion on this, and the history for --build-id in lld/ELF probably finds the same discussion for ELF.

int3 added a comment.Nov 17 2020, 6:27 AM

I used MD5 since that's what ld64 does. I hadn't looked at what lld-ELF/COFF were doing for this though, so thanks for the pointer! The only possible concern I can think of is the fact that the UUID version tags as specified in its RFC only provide for MD5 or SHA1 hashing, and ld64 seemed to think it important that we conform to this RFC. But I'm hard pressed to see how it would be an issue if we used a different hashing algorithm but tagged it as MD5. I'm inclined to just leave a TODO for now and come back later when we are tuning for performance