This is an archive of the discontinued LLVM Phabricator instance.

[lld][X86] Restore gotEntrySize.
ClosedPublic

Authored by hvdijk on May 14 2021, 9:45 AM.

Details

Summary

D62727 removed GotEntrySize and GotPltEntrySize with a comment that they are always equal to wordsize(), but that is not entirely true: X32 has a word size of 4, but needs 8-byte GOT entries. This restores gotEntrySize for both, adjusted for current naming conventions, but defaults it to config->wordsize to keep things simple for architectures other than x86_64.

This partially reverts D62727.

Diff Detail

Event Timeline

hvdijk created this revision.May 14 2021, 9:45 AM
hvdijk requested review of this revision.May 14 2021, 9:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 9:45 AM
MaskRay requested changes to this revision.May 14 2021, 9:57 AM

We shouldn't need both GotEntrySize and GotPltEntrySize. One is sufficient.

lld/test/ELF/x86-x32-plt.s
2

Don'y copy x86-64-plt.s. Newer tests prefer #

Consider riscv-plt.s

8

Most tests llvm-readelf should be used. Its output is succinct.

60

This line is not useful.

63

The comment after the instruction is useful. It computes the target address.

This revision now requires changes to proceed.May 14 2021, 9:57 AM

We shouldn't need both GotEntrySize and GotPltEntrySize. One is sufficient.

Alright, will update to have just GotEntrySize.

lld/test/ELF/x86-x32-plt.s
2

I'd like to avoid unnecessary differences between this and x86-64-plt.s since they both basically test the same thing, just for slightly different targets. Will adjust the other test as well to keep them similar, same for the other comments.

MaskRay added inline comments.May 14 2021, 10:07 AM
lld/test/ELF/x86-x32-plt.s
2

please don't copy x86-64-plt.s. i'll improve x86-64-plt.s shortly.

Hope the scheme here should be generic enough to support aarch64-ilp32 and Morello.

hvdijk updated this revision to Diff 345569.May 14 2021, 2:43 PM
hvdijk retitled this revision from [lld][X86] Restore gotEntrySize and gotPltEntrySize. to [lld][X86] Restore gotEntrySize..
hvdijk edited the summary of this revision. (Show Details)

I'm assuming you meant don't base the test on the old x86-64-plt.s, but the new is fine. Done now, and removed gotPltEntrySize.

Is x86-x32-plt.s NFC for now and you'll create another patch to fix x32?

lld/test/ELF/x86-x32-plt.s
7

You can drop %t.so testing.

I added more to x86-64-plt.s because there needs a test to check additional target agnostic properties.

x86-x32-plt.s doesn't need to replicate the testing.

Is x86-x32-plt.s NFC for now and you'll create another patch to fix x32?

This isn't NFC, this fixes the test in here. Other x32 problems remain but they're separate issues and I'll create separate patches for them.

I'll take the -shared things out of the test.

hvdijk updated this revision to Diff 345626.May 15 2021, 5:04 AM

Removed the shared object testing.

MaskRay accepted this revision.May 16 2021, 2:42 PM
This revision is now accepted and ready to land.May 16 2021, 2:42 PM
This revision was automatically updated to reflect the committed changes.

Hope the scheme here should be generic enough to support aarch64-ilp32 and Morello.

For Morello, there is already a similar downstream patch that introduces gotEntrySize as separate from word-size (128-bit and 64-bit respectively) so yes it can work. I'm not familiar with an existing LLD implementation of aarch64-ilp32 but I can't see any reason why it wouldn't work.