This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Support synthesizing __TEXT,__init_offsets
ClosedPublic

Authored by BertalanD on Aug 30 2022, 8:05 AM.

Details

Summary

This section stores 32-bit TEXT segment offsets of initializer
functions, and is used instead of
mod_init_func when chained fixups
are enabled.

Storing the offsets lets us avoid needing to fix up the initializers.

Diff Detail

Event Timeline

BertalanD created this revision.Aug 30 2022, 8:05 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: ributzka. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
BertalanD requested review of this revision.Aug 30 2022, 8:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 8:05 AM
thakis accepted this revision.Aug 30 2022, 8:35 AM
thakis added a subscriber: thakis.

Looks great!

lld/MachO/Driver.cpp
1110

Looks like ld64 also adds the arg to -init (we don't implement this flag yet, but should probably have a FIXME somewhere to add it to initOffsets once we do?)

1440

I wanted to ask if this is enabled by default at some output os version, but it looks like it's on by default if chained fixups are on, which are on by default at some output os version. So D132560 will also enable this, I imagine.

lld/MachO/MarkLive.cpp
280

Should we _not_ enqueue S_MOD_INIT_FUNC_POINTERS here if we're doing this transform, since initOffsets is what produces lifeness in that case?

…oh, I see, we don't even put S_MOD_INIT_FUNC_POINTERS sections into inputSections in that case, so this will never happen.

Should this loop assert(!config->emitInitOffsets || sectionType(isec->getFlags()) != S_MOD_INIT_FUNC_POINTERS) to make that more clear, maybe?

lld/MachO/Writer.cpp
1136

Very nit: I'd appreciate it if you could rename the existing two setup() methods to setUp() in a preparatory commit (just land, no need for review), and then call this new one setUp too. ("setup" is a noun, "to set up" is a verb, and methods are named after verbs.)

lld/test/MachO/init-offsets.s
16

Good test!

llvm/lib/MC/MCSectionMachO.cpp
65

I wanted to say something about this, but apparently it's local style in this file O_o

This revision is now accepted and ready to land.Aug 30 2022, 8:35 AM
BertalanD marked an inline comment as done.Aug 30 2022, 8:42 AM
BertalanD added inline comments.
lld/MachO/Writer.cpp
1136

sgtm

llvm/lib/MC/MCSectionMachO.cpp
65

I'm considering adding a separate patch that cleans these up. Apparently it was uglified because some old GCC version didn't like implicitly converting a string literal to StringLiteral. (https://github.com/llvm/llvm-project/commit/a9df9414030f6cbec511a8cef941998a268b9c6e)

I assume we're way past the "apparently GCC still doesn't understand C++11" situation (at least since the required was bumped to C++17).

BertalanD marked an inline comment as done.Aug 30 2022, 8:46 AM
BertalanD added inline comments.
llvm/lib/MC/MCSectionMachO.cpp
65

And the FIXME part should be better described as "this section is synthesized by the linker, so the assembler does not need to know about it". That's a bit long for an inline comment though...

thakis added inline comments.Aug 30 2022, 9:32 AM
llvm/lib/MC/MCSectionMachO.cpp
65

/* linker-synthesized */ maybe?

BertalanD updated this revision to Diff 456704.Aug 30 2022, 9:47 AM
BertalanD marked 6 inline comments as done.Aug 30 2022, 9:51 AM

Addressed review comments.

I'm going to wait with landing this until tomorrow in case someone wants to make some suggestions.

lld/MachO/Driver.cpp
1440

That's correct.

This revision was landed with ongoing or failed builds.Aug 31 2022, 1:22 AM
This revision was automatically updated to reflect the committed changes.