Page MenuHomePhabricator

[lld-macho] Add support for -image_base, -static, -pagezero_size, and -version_load_command
Needs RevisionPublic

Authored by Link1J on Jan 30 2022, 10:22 AM.

Details

Reviewers
MaskRay
int3
Group Reviewers
Restricted Project
Summary

This adds the -image_base, -static, -pagezero_size, and -version_load_command.

-image_base, -static, and -pagezero_size commonly used for kernel development. So I don't know how useful these will be to most people, but I do believe that having lld be a more complete replacement for the Apple linker is a good thing.

-image_base sets the virtual address of the first section, changing the starting address the binary is loaded at. Basically the same thing as /BASE from link and lld-link.

-pagezero_size changes the __PAGEZERO size, removing that segment if it is set to zero. I personally have no idea what __PAGEZERO is for, but it isn't useful with a custom image base.

-static does alot. It changes the entry point command from LC_MAIN to LC_UNIXTHREAD. It also stops the LC_BUILD_VERSION, LC_VERSION_MIN_*, LC_LOAD_DYLINKER, LC_DATA_IN_CODE, and LC_FUNCTION_STARTS commands from being added to the binary. It appears that -function_starts and -version_load_command readds their respective commands to the binary, so -static only changes the default for commands.

-version_load_command adds LC_BUILD_VERSION or LC_VERSION_MIN_* when -static is used. It got added because it was easy to add as I was doing -static. I only learnt about -version_load_command from reading the Makefiles for the XNU kernel used in macOS. Maybe in the future, it can be extended to control which version command gets added, but that is far beyond what I intended to do.

Diff Detail

Event Timeline

Link1J created this revision.Jan 30 2022, 10:22 AM
Link1J requested review of this revision.Jan 30 2022, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2022, 10:22 AM
Link1J updated this revision to Diff 404396.Jan 30 2022, 10:47 AM
Link1J updated this revision to Diff 404398.Jan 30 2022, 10:56 AM

For some reason, I am ending up with diffs that use CRLF instead of LF.

Link1J updated this revision to Diff 404406.Jan 30 2022, 12:14 PM

Thanks for the contribution :)

You should add tests for all of these changes. Our tests are in lld/test/MachO. If you haven't worked with LLVM's test system before, there's overviews of lit, FileCheck, and a general testing guide. check-lld-macho is the build target for our tests.

I'll let others take a look at the code changes in detail, but the high-level idea seems good to me.

int3 added a subscriber: int3.Jan 30 2022, 5:42 PM

Neat, thanks for implementing all these! As @smeenai said, we definitely need tests :)

Also, could you break up the diff to make review easier? I think each flag merits its own diff

lld/MachO/Config.h
100

we haven't been super consistent about this, but it would be nice to keep the Configuration struct organized by field type. E.g. addLoadVersion could go here

lld/MachO/Writer.cpp
49

from what I understand, -image_base should control the address of the mach header, but setting addr here means that we're setting the start address of __PAGEZERO instead, which doesn't seem right

Also, I wonder what happens when -image_base points to an address within the __PAGEZERO segment...

316

no need for llvm::

Link1J added inline comments.Jan 31 2022, 7:22 AM
lld/MachO/Writer.cpp
49

From the testing I had done, I thought it was setting the start address for __PAGEZERO. I did more testing, and found it is not.

And if -image_base is inside the __PAGEZERO, it prints out the section list, says the last non __LINKEDIT section overlaps the __PAGEZERO, and doesn't output anything. I think we could do better, because ld64 error message doesn't even say error anywhere in it.

Also, could you break up the diff to make review easier? I think each flag merits its own diff

As in submitting multiple revisions for review? Sorry if the question seems dumb, but this is the first time I have use Phabricator.

thakis added a subscriber: thakis.Jan 31 2022, 10:44 AM

Also, could you break up the diff to make review easier? I think each flag merits its own diff

As in submitting multiple revisions for review? Sorry if the question seems dumb, but this is the first time I have use Phabricator.

Right.

MaskRay added inline comments.Jan 31 2022, 10:51 AM
lld/MachO/Config.h
192

initialize to zero

lld/MachO/Writer.cpp
856

Prefer if (xxx) to if (!xxx) if both then and else branches are needed.

lld/include/lld/Common/Args.h
34

Default is a leftover when the variable naming switches to lowerCase. Perhaps defaultVal

int3 requested changes to this revision.Mar 16 2022, 2:34 PM

clearing queue

This revision now requires changes to proceed.Mar 16 2022, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 2:34 PM