Page MenuHomePhabricator

[lld-macho] Add -pagezero_size
ClosedPublic

Authored by Link1J on Feb 1 2022, 12:17 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG31626cc111c0: [lld-macho] Add -pagezero_size
Summary

Adds -pagezero_size. -pagezero_size commonly used for kernel development.
-pagezero_size changes the __PAGEZERO size, removing that segment if it is set to zero.

One of the four flags from D118570: [lld-macho] Add support for -image_base, -static, -pagezero_size, and -version_load_command
Now with error messages and tests.

Diff Detail

Event Timeline

Link1J created this revision.Feb 1 2022, 12:17 PM
Link1J requested review of this revision.Feb 1 2022, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 12:17 PM
oontvoo added inline comments.
lld/MachO/Target.h
38–39 ↗(On Diff #405042)
lld/test/MachO/pagesize-zero.s
1 ↗(On Diff #405042)

could you group all three of these files into the one test file?

int3 added inline comments.Feb 1 2022, 1:19 PM
lld/MachO/Driver.cpp
656–657

Hm, this seems like an ld64 implementation error. Personally I think it makes more sense to round it to the page size...

658

nit: would be nice to use the isAligned helper for a bit more explicitness -- isAligned(Align(target->getPageSize()), pagezeroSize)

lld/test/MachO/pagesize-zero.s
1 ↗(On Diff #405042)

+1

lld/test/MachO/pagezero-misaligned.s
3 ↗(On Diff #405042)

we should test the 32-bit case here too (regardless of whether we use 4KiB or the actual page size to round down).

the only 32-bit target we properly support is arm64_32, so that's what I would use here. (You can use %lld-watchos instead of %lld to link for that target.)

Link1J added inline comments.Feb 1 2022, 5:26 PM
lld/MachO/Target.h
38–39 ↗(On Diff #405042)

I know that is how it should be, but it doesn't seem to build correctly on Linux.
Here is the error message that appears

ld.lld: error: undefined symbol: lld::macho::ILP32::pageZeroSize
>>> referenced by Optional.h:0 (/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/ADT/Optional.h:0)
>>>               ARM.cpp.o:(lld::macho::createARMTargetInfo(unsigned int)) in archive lib/liblldMachO.a
>>> referenced by Optional.h:0 (/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/ADT/Optional.h:0)
>>>               ARM64_32.cpp.o:(lld::macho::createARM64_32TargetInfo()) in archive lib/liblldMachO.a
 
ld.lld: error: undefined symbol: lld::macho::LP64::pageZeroSize
>>> referenced by Optional.h:0 (/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/ADT/Optional.h:0)
>>>               ARM64.cpp.o:(lld::macho::createARM64TargetInfo()) in archive lib/liblldMachO.a
>>> referenced by Optional.h:0 (/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/ADT/Optional.h:0)
>>>               X86_64.cpp.o:(lld::macho::createX86_64TargetInfo()) in archive lib/liblldMachO.a

Gotten from https://buildkite.com/llvm-project/premerge-checks/builds/76612#af4b783f-6fe5-4a09-a98e-9b180f113190/467-3299

int3 added inline comments.Feb 2 2022, 10:10 AM
lld/MachO/Target.h
136 ↗(On Diff #405042)

re the compilation bug above: what if you changed this from a constexpr to a const?

Link1J added inline comments.Feb 2 2022, 12:07 PM
lld/MachO/Target.h
136 ↗(On Diff #405042)

Changing constexpr to const doesn't work. But adding inline does work.
It seems the C++14 spec doesn't like doing what we want to do, because it fails on gcc with -std=c++14.
MSVC is the only one that doesn't care about the C++ version, and so it always works with MSVC and clang-cl.

Link1J added inline comments.Feb 5 2022, 3:29 PM
lld/MachO/Driver.cpp
656–657

Well, ld64 has some interesting behavior because of what it does.
If you are to set -pagezero_size to 0x1000 on ARM64, but round up to page size (16KB).
If you are to set -pagezero_size to 0x1001 on ARM64, it will report the rounding down, but round up to page size.

I am going to make it operate on page size, over whatever ld64 does, so that it makes a lot more sense. So it will round down to page size, and never make it larger then expected.

Link1J updated this revision to Diff 406218.Feb 5 2022, 4:30 PM
int3 added a comment.Feb 5 2022, 8:38 PM

Nice, almost there!

Could you add a description of what -pagezero_size does to the commit message? I know you linked to your original diff, but it would be nice to avoid having people click through to find it :)

lld/MachO/Driver.cpp
1142

some ld64 engineers may read this, let's not say 'dumb' :)... 'weird' is fine

1145–1146

how about printing out the new value? like instead of rounding down to realign, we could have rounding down to 0x10000000 or something

(we can print out the size int easily using Twine::utohexstr)

lld/test/MachO/pagezero.s
11

IMO it would make the test clearer if we used llvm-readobj --macho-segment instead. Then we could test the segment size directly as follows:

# CHECK:        Name: __PAGEZERO
# CHECK-NEXT:   Size:
# CHECK-NEXT:   vmaddr: 0x0

also, we can reuse the same check lines if we use FileCheck's numeric substitutions (https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks):

# RUN: llvm-readobj --macho-segment %t/arm64_32 | FileCheck %s -D#SIZE=0x0
# CHECK:        Name: __PAGEZERO
# CHECK-NEXT:   Size:
# CHECK-NEXT:   vmaddr: 0x[[#%x,SIZE]]
13

nit: use - instead of _ for CHECK- lines

29

nit: most other tests use this naming scheme

int3 added inline comments.Feb 5 2022, 8:40 PM
lld/MachO/Driver.cpp
656–657

lol that's pretty funny behavior. yeah I agree with your choice here

Link1J updated this revision to Diff 406247.Feb 6 2022, 7:03 AM
Link1J marked 6 inline comments as done.
Link1J edited the summary of this revision. (Show Details)
Link1J updated this revision to Diff 406250.Feb 6 2022, 7:25 AM
int3 accepted this revision.Feb 6 2022, 9:38 AM

lgtm. Do you need me to commit it for you? If so, let me know what name and email I should use :)

lld/MachO/Driver.cpp
1142
This revision is now accepted and ready to land.Feb 6 2022, 9:38 AM

Can you please commit it for me.
Name: Jared Irwin
Email: jrairwin@sympatico.ca

This revision was automatically updated to reflect the committed changes.