This is an archive of the discontinued LLVM Phabricator instance.

lld: Default image base address to 0x200000 on x86-64
ClosedPublic

Authored by emaste on Nov 23 2016, 6:17 AM.

Details

Summary

Align to the least common multiple of the large page size (known as a superpage or huge page) available on common platforms. Non-PAE x86 supports 4MB pages and x86-64 and arm64 support 2MB pages. FreeBSD automatically promotes large, superpage-aligned allocations, and this default image base address is consistent with GNU ld and gold.

Test updates are incomplete. I started with a sed replacement to update some of them, and am iterating through the remaining failures. Some tests need more investigation e.g. ELF/aarch64-prel32.s is producing a relocation out of range error.

Assuming there are no objections to this change in principle I will continue with the test updates.

Diff Detail

Repository
rL LLVM

Event Timeline

emaste updated this revision to Diff 79061.Nov 23 2016, 6:17 AM
emaste retitled this revision from to lld: Default image base address to 0x400000.
emaste updated this object.
emaste added reviewers: ruiu, rafael.
emaste added a subscriber: llvm-commits.
emaste updated this revision to Diff 79070.Nov 23 2016, 7:03 AM
emaste retitled this revision from lld: Default image base address to 0x400000 to lld: Default image base address to 0x200000 on x86-64.

Incorporate feedback from Rafael:

  • Use 2MB instead of 4MB
  • Apply only to x86-64 for now
emaste added inline comments.Nov 23 2016, 7:04 AM
test/ELF/gnu-ifunc-i386.s
99–100 ↗(On Diff #79070)

Shall I change these cases to {{.*}} first?

Oops, I applied the change to x86, not x86-64. Will add new patch shortly.

rafael added inline comments.Nov 23 2016, 7:07 AM
ELF/Target.cpp
317 ↗(On Diff #79070)

Do update the comment on the .h file too :-)

This is updating i386, not x86_64.

grimar added a subscriber: grimar.Nov 23 2016, 7:19 AM
emaste updated this revision to Diff 79081.Nov 23 2016, 7:44 AM

Apply to x86-64

emaste added inline comments.Nov 23 2016, 7:51 AM
ELF/Target.h
71–72 ↗(On Diff #79081)

The minimum is still 0x10000 so leaving the rest of the comment intact makes sense to me.

test/ELF/build-id.s
47 ↗(On Diff #79081)

I'm not sure why these were checking only the first byte of the build-id - I can change back to doing that if desired

test/ELF/eh-frame-hdr-icf.s
9 ↗(On Diff #79081)

need to update whitespace

11 ↗(On Diff #79081)

and here

test/ELF/eh-frame-hdr.s
67–77 ↗(On Diff #79081)

Addresses in these comments need to be checked/updated

test/ELF/gnu-ifunc.s
98–100 ↗(On Diff #79081)

should I switch these to {{.*}} in a first commit?

test/ELF/gotpc-relax-nopic.s
18–26 ↗(On Diff #79081)

Should I switch these to {{.*}}

test/ELF/merge.s
38–39 ↗(On Diff #79081)

Comments need to be updated

test/ELF/no-inhibit-exec.s
9 ↗(On Diff #79081)

{{.*}} ?

test/ELF/relocation-copy.s
62–67 ↗(On Diff #79081)

{{.*}} ?

test/ELF/relocation-local.s
27 ↗(On Diff #79081)

Change this to maintain a negative offset?

rafael accepted this revision.Nov 23 2016, 8:14 AM
rafael edited edge metadata.

LGTM

test/ELF/eh-frame-hdr.s
67 ↗(On Diff #79081)

It is a pity "llvm-objdump -dwarf=frames" is broken :-(

test/ELF/gotpc-relax-nopic.s
18–26 ↗(On Diff #79081)

Yes please.

test/ELF/no-inhibit-exec.s
9 ↗(On Diff #79081)

Yes

test/ELF/relocation-copy.s
62–67 ↗(On Diff #79081)

Yes.

This revision is now accepted and ready to land.Nov 23 2016, 8:14 AM
ruiu accepted this revision.Nov 23 2016, 9:28 AM
ruiu edited edge metadata.

LGTM

ELF/Target.h
67–68 ↗(On Diff #79081)

nit: freebsd -> Freebsd, linux -> Linux

emaste added inline comments.Nov 23 2016, 9:39 AM
test/ELF/gotpc-relax-nopic.s
15 ↗(On Diff #79081)

Another comment to update

test/ELF/relocation-local.s
27 ↗(On Diff #79081)

This was originally introduced in D12978 / rL248196 and I don't see a reason it must be negative.

emaste updated this revision to Diff 79112.Nov 23 2016, 9:42 AM
emaste edited edge metadata.
  • Rebase after rL287778 {{.*}}
  • Incorporate review feedback and update a few missed address comments
This revision was automatically updated to reflect the committed changes.