Page MenuHomePhabricator

Align AArch64 and i386 image base to superpage
ClosedPublic

Authored by dim on Aug 4 2018, 6:03 AM.

Details

Summary

As for x86_64, the default image base for AArch64 and i386 should be aligned to a superpage appropriate for the architecture.

On AArch64, this is 2 MiB, on i386 it is 4 MiB.

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Aug 4 2018, 6:03 AM
dim added a comment.Aug 4 2018, 6:04 AM

Note that we have already committed this in FreeBSD, see https://reviews.freebsd.org/D16385 and https://reviews.freebsd.org/rS337282.

grimar added a reviewer: ruiu.Aug 4 2018, 6:51 AM
grimar added a comment.Aug 4 2018, 6:54 AM

I do not know if this change is OK or not,
but for the record, 48 tests are failing with this patch applied:

35> Failing Tests (48):
35> lld :: ELF/aarch64-abs16.s
35> lld :: ELF/aarch64-abs32.s
35> lld :: ELF/aarch64-call26-thunk.s
35> lld :: ELF/aarch64-condb-reloc.s
35> lld :: ELF/aarch64-copy.s
35> lld :: ELF/aarch64-copy2.s
35> lld :: ELF/aarch64-cortex-a53-843419-large.s
35> lld :: ELF/aarch64-cortex-a53-843419-recognize.s
35> lld :: ELF/aarch64-data-relocs.s
35> lld :: ELF/aarch64-gnu-ifunc-plt.s
35> lld :: ELF/aarch64-gnu-ifunc.s
35> lld :: ELF/aarch64-jump26-thunk.s
35> lld :: ELF/aarch64-lo12-alignment.s
35> lld :: ELF/aarch64-prel16.s
35> lld :: ELF/aarch64-prel32.s
35> lld :: ELF/aarch64-relocs.s
35> lld :: ELF/aarch64-thunk-section-location.s
35> lld :: ELF/aarch64-tls-gdie.s
35> lld :: ELF/aarch64-tls-gdle.s
35> lld :: ELF/aarch64-tls-ie.s
35> lld :: ELF/aarch64-tls-iele.s
35> lld :: ELF/aarch64-tls-le.s
35> lld :: ELF/aarch64-tlsld-ldst.s
35> lld :: ELF/aarch64-tstbr14-reloc.s
35> lld :: ELF/aarch64-undefined-weak.s
35> lld :: ELF/basic-aarch64.s
35> lld :: ELF/basic32.s
35> lld :: ELF/gc-sections-implicit-addend.s
35> lld :: ELF/gnu-ifunc-i386.s
35> lld :: ELF/gnu-ifunc-plt-i386.s
35> lld :: ELF/got-i386.s
35> lld :: ELF/got32-i386.s
35> lld :: ELF/got32x-i386.s
35> lld :: ELF/i386-pc8-pc16-addend.s
35> lld :: ELF/i386-retpoline-nopic.s
35> lld :: ELF/map-file-i686.s
35> lld :: ELF/plt-aarch64.s
35> lld :: ELF/plt-i686.s
35> lld :: ELF/relocation-b-aarch64.test
35> lld :: ELF/relocation-copy-i686.s
35> lld :: ELF/relocation-i686.s
35> lld :: ELF/shared.s
35> lld :: ELF/static-with-export-dynamic.s
35> lld :: ELF/tls-i686.s
35> lld :: ELF/tls-opt-gdiele-i686.s
35> lld :: ELF/tls-opt-i686.s
35> lld :: ELF/tls-opt-iele-i686-nopic.s
35> lld :: ELF/undef-with-plt-addr-i686.s

dim added a comment.Aug 4 2018, 11:38 AM

Looking at this a bit more, I see that it is unconditionally setting the DefaultImage base for all platforms, which cannot be right. I didn't notice this when it was committed into FreeBSD, but it doesn't matter there, of course. I'm not sure if there is a good way to override this setting on a per-platform basis. Maybe just if (os == FreeBSD) DefaultImageBase=x?

ruiu added a comment.Aug 6 2018, 10:41 AM

Looking at this a bit more, I see that it is unconditionally setting the DefaultImage base for all platforms, which cannot be right. I didn't notice this when it was committed into FreeBSD, but it doesn't matter there, of course. I'm not sure if there is a good way to override this setting on a per-platform basis. Maybe just if (os == FreeBSD) DefaultImageBase=x?

Is there any reason that this value doesn't work for other operating systems? I guess the new image base values work everywhere. If that's the case, I'd change the default so that we have less moving parts in the linker.

dim added a comment.Aug 7 2018, 10:42 PM

Looking at this a bit more, I see that it is unconditionally setting the DefaultImage base for all platforms, which cannot be right. I didn't notice this when it was committed into FreeBSD, but it doesn't matter there, of course. I'm not sure if there is a good way to override this setting on a per-platform basis. Maybe just if (os == FreeBSD) DefaultImageBase=x?

Is there any reason that this value doesn't work for other operating systems? I guess the new image base values work everywhere. If that's the case, I'd change the default so that we have less moving parts in the linker.

I'm not sure if it will work on all OSes, I can't test them all. In any case, I'll update the test cases first, then update this review.

markj added a subscriber: markj.Aug 17 2018, 9:30 AM

I'll just note that there is an identical default in X86_64.cpp. Also see the last comment by alc here: https://reviews.freebsd.org/D16385 .

dim updated this revision to Diff 162886.Aug 28 2018, 9:06 AM

Update all tests to match current output.

jfb added a comment.Aug 28 2018, 9:44 AM

Please change the patch's title and description if your intent is to affect more than just FreeBSD.

dim retitled this revision from On FreeBSD, align AArch64 and i386 image base to superpage to Align AArch64 and i386 image base to superpage.Aug 28 2018, 1:03 PM
dim edited the summary of this revision. (Show Details)
ruiu added a comment.Aug 28 2018, 7:01 PM

Oh, I didn't notice that you are trying to make a change for i386, not for x86-64. What is your motivation to use superpages for 32-bit applications? I believe that the applications you want to use superpages are large programs and naturally be 64-bit, so I wonder if this change is actually useful.

dim added a comment.Aug 29 2018, 3:48 AM

Oh, I didn't notice that you are trying to make a change for i386, not for x86-64. What is your motivation to use superpages for 32-bit applications? I believe that the applications you want to use superpages are large programs and naturally be 64-bit, so I wonder if this change is actually useful.

I think it is for consistency, and for efficiency in the virtual memory manager, but @markj is probably better at explaining it. :)

markj added a comment.Aug 29 2018, 7:57 AM

Oh, I didn't notice that you are trying to make a change for i386, not for x86-64. What is your motivation to use superpages for 32-bit applications? I believe that the applications you want to use superpages are large programs and naturally be 64-bit, so I wonder if this change is actually useful.

All executables benefit from being loaded at a superpage-aligned address, assuming that the RO segment is large enough to fill out at least one superpage. The following segments can then be suitably aligned using -z max-page-size. Further, when an application faults on a shared segment (e.g., the text section of an executable), if the fault region is mapped as a superpage in another process, FreeBSD will attempt to create a new superpage mapping in the faulting application, potentially eliding many faults on 4KB pages. This is all mostly orthogonal to the application-specific uses of superpages and applies to i386 as well as x86-64.

dim added a comment.Sep 16 2018, 1:40 AM

So, any more actions to take, or can I commit this?

ruiu added a comment.Sep 17 2018, 7:46 AM

I'm fine with this. For lld, we tried to not cargo-cult default image base values, and the current default values are chosen based on the rationale described in Target.h. If some other value is better than the current default, we shouldn't too hesitate to change that.

Since this patch changes the default image base for AArch64, I'd wait for Peter Smith's LGTM.

ELF/Arch/X86.cpp
64 ↗(On Diff #162886)

I'd s/non-PAE large/4 MiB/ as I don't remember the number.

I've added Stephen Hines as a reviewer to see if this affects Android in a significant way. I'd like to run this by a few people in the OS community and also check that are no adverse code/file size increases as these could be significant on low-end mobile devices. Will hopefully get back to you soon.

ruiu added a comment.Sep 17 2018, 8:45 AM

In theory this change shouldn't affect the amount of memory used by an application, since it affects only the virtual address space, no? The only bad scenario I can think about this patch is, if your i386 application barely fits within the 2 GiB/4GiB address space, increasing the base address a few megabyte could cause a link failure. But I don't think that's a realistic scenario.

+Ryan in case there is anything here that could affect Bionic loading from these pages.

In theory this change shouldn't affect the amount of memory used by an application

I believe that is the case, yes.

In the past I've worked on a proprietary i386 application that used as much address space as it could get, but this is a very unusual case, which can be addressed on an as-needed basis, and is largely irrelevant today as such applications transitioned to x86_64 years ago.

Does FreeBsd use 4Kb granules for the page size or the LLD default of 64Kb. From what I can see from the MMU documentation when 64Kb pages are used the equivalent of the Super Page size is 512MiB and not 2 MiB? If I'm right then aligning to 2MiB is only going to be beneficial when 4Kb pages are used. On the assumption that it doesn't do much harm to people using 64Kb pages I don't have any objections in changing it though. Aligning to 512MiB for 64kb pages sounds a bit extreme though.

To summarise if Android/Bionic are happy that this isn't going to be a problem then I'm ok with it.

ELF/Arch/AArch64.cpp
73 ↗(On Diff #162886)

My understanding is that when 64k pages are used (see DefaultMaxPageSize) the size of the huge/super page is 512MiB. Aligning to 2MiB on AArch64 may not be effective for the default 64kb pages.

At the moment the most official reference I can find is in the Arm ARM VMSAv8-64 translation table level 0, level 1, and level 2 descriptor formats
https://static.docs.arm.com/ddi0487/ca/DDI0487C_a_armv8_arm.pdf

markj added a comment.Sep 18 2018, 6:16 PM

Does FreeBsd use 4Kb granules for the page size or the LLD default of 64Kb. From what I can see from the MMU documentation when 64Kb pages are used the equivalent of the Super Page size is 512MiB and not 2 MiB? If I'm right then aligning to 2MiB is only going to be beneficial when 4Kb pages are used. On the assumption that it doesn't do much harm to people using 64Kb pages I don't have any objections in changing it though. Aligning to 512MiB for 64kb pages sounds a bit extreme though.

Currently we don't have any support for anything other than 4KB pages on aarch64. This change doesn't have any effect on 64KB alignment so I can't see why anyone using 64KB pages would be negatively affected.

+Ryan in case there is anything here that could affect Bionic loading from these pages.

AFAIK, this patch isn't a problem for Android. It looks like it could only affect NDK-built non-PIE executables running on 32-bit x86 Jelly Bean or KitKat.

It looks like the ImageBase only matters for non-PIE executables, and the Bionic loader rejects those on android-21 (L) and up for security reasons. The oldest Android OS that the current NDK supports is android-16, which allows PIE executables but can also still run non-PIE executables. L is the first 64-bit release, so this patch can't affect arm64 Android. The NDK Clang driver turns PIE on by default for android-16 and up, but someone could turn it off or use a build system that doesn't set the android-16 API level.

bfd and gold use a default image base of 0x08048000 on x86 (a bit over 128MB), but AFAICT none of the rationales I found for that make sense today. I experimented with various bases from 0 up to 3GB on android-16 and android-19 emulators, and everything seems to work.

peter.smith accepted this revision.Sep 19 2018, 8:04 AM

Thanks for the updates. LGTM.

This revision is now accepted and ready to land.Sep 19 2018, 8:04 AM
ruiu accepted this revision.Sep 19 2018, 8:17 AM

LGTM

This revision was automatically updated to reflect the committed changes.