Page MenuHomePhabricator

[lld-macho] Implement and test resolution of common symbols
ClosedPublic

Authored by int3 on Aug 31 2020, 11:11 PM.

Details

Reviewers
gkm
Group Reviewers
Restricted Project
Commits
rGcd7cb0c30305: [lld-macho] Implement and test resolution of common symbols
Summary

Handle the case where there are both common and non-common definitions
of the same symbol. Add a bunch of tests to ensure compatibility with ld64.

Diff Detail

Event Timeline

int3 created this revision.Aug 31 2020, 11:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2020, 11:11 PM
int3 requested review of this revision.Aug 31 2020, 11:11 PM
int3 updated this revision to Diff 289995.Sep 4 2020, 10:46 AM

test common and regular symbols together in an archive

int3 updated this revision to Diff 292670.Sep 17 2020, 5:16 PM

fix test

gkm added a subscriber: gkm.Sep 22 2020, 7:15 PM
gkm added inline comments.
lld/MachO/SymbolTable.cpp
85

s/</<=/. I assume that replaceSymbol implies extra work, so why do that for equal-sized data?

90–91

Tangent: I noticed that DSOHandle is not part of SymbolUnion. Should it be?

lld/test/MachO/common-symbol-resolution.s
69–73

I don't see a use for this order file.

int3 marked 3 inline comments as done.Sep 23 2020, 8:26 AM
int3 added inline comments.
lld/MachO/SymbolTable.cpp
85

it's to change the alignment. (See also discussion in D86909)

90–91

oops yeah. I'll add it as part of D86909 since I need to add CommonSymbol there too

lld/test/MachO/common-symbol-resolution.s
69–73

oops...

int3 updated this revision to Diff 293756.Sep 23 2020, 8:34 AM
int3 marked 3 inline comments as done.

use order file

gkm accepted this revision.Sep 23 2020, 5:29 PM
gkm added inline comments.
lld/MachO/SymbolTable.cpp
85

Let me be sure I understand:

  • You coded it this way for sake of consistency with ld64.
  • A later equal-sized common with smaller alignment will supersede the earlier with greater alignment.
  • This seems wrong, but whatever. Call it bug-for-bug compatible.
  • It must be a bug of little consequence, otherwise it would be fixed. Maybe we should fix it anyway and see if someone complains!
This revision is now accepted and ready to land.Sep 23 2020, 5:29 PM
int3 added inline comments.Sep 23 2020, 5:31 PM
lld/MachO/SymbolTable.cpp
85

yup that's right, I was going for bug4bug compatibility. From what I understand, common symbols are being phased out in modern projects, so it's probably not too important anyway

This revision was automatically updated to reflect the committed changes.
omjavaid reopened this revision.Sep 24 2020, 12:28 AM
omjavaid added a subscriber: omjavaid.

Temporarily reverted this change as it predecessor broke LLDB build on Arm. https://reviews.llvm.org/D86909
http://lab.llvm.org:8011/builders/lldb-arm-ubuntu/builds/4409

This revision is now accepted and ready to land.Sep 24 2020, 12:28 AM