This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Fix TLV data initialization
ClosedPublic

Authored by int3 on Jan 8 2021, 12:04 PM.

Details

Reviewers
thakis
Group Reviewers
Restricted Project
Commits
rGdaaaed6bb890: [lld-macho] Fix TLV data initialization
Summary

We were mishandling the case where both __tbss and __thread_data sections were
present.

TLVP relocations should be encoded as offsets from the start of __thread_data,
even if the symbol is actually located in __thread_bss. Previously, we were
writing the offset from the start of the containing section, which doesn't
really make sense since there's no way tlv_get_addr() can know which section a
given tlv$init symbol is in at runtime.

In addition, this patch ensures that we place __thread_data immediately before
__thread_bss. This is what ld64 does, likely for performance reasons. Zerofill
sections must also be at the end of their segments; we were already doing this,
but now we ensure that __thread_bss occurs before __bss, so that it's always
possible to have it contiguous with __thread_data.

Fixes llvm.org/PR48657.

Diff Detail

Event Timeline

int3 requested review of this revision.Jan 8 2021, 12:04 PM
int3 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2021, 12:04 PM
int3 edited the summary of this revision. (Show Details)Jan 8 2021, 12:05 PM
int3 added inline comments.
lld/MachO/Writer.cpp
590–593

ld64 has logic that does this, but I have no idea how we can actually test it -- it doesn't look like it's possible to create arbitrary S_ZEROFILL sections from assembly, at least if I'm reading MCSectionMachO.cpp correctly. I could remove this / replace it with an assert if anyone cares.

smeenai added inline comments.
lld/MachO/Writer.cpp
590–593

There's a .zerofill directive ... does that do what you want?

int3 marked an inline comment as done.Jan 8 2021, 12:28 PM
int3 added inline comments.
lld/MachO/Writer.cpp
590–593

ooh yeah it does. I had been looking for modifiers to the .section directive... thanks!

int3 updated this revision to Diff 315491.Jan 8 2021, 12:39 PM
int3 marked an inline comment as done.

update bss.s test

thakis accepted this revision.Jan 8 2021, 1:38 PM
thakis added a subscriber: thakis.

Very nice!

lld/MachO/InputSection.cpp
50

fwiw I think of it as a single thread-local data memory area – it's kind of more like a segment than a section I suppose, and it implicitly consists of DATA,thread_data and DATA,thread_bss (and, per dyld code, everything in between -- which is why we don't want anything in between, since that'd uselessly waste memory)

lld/MachO/InputSection.h
78

very nit: swap order of ZEROFILL and REGULAR since REGULAR comes first in the TLV area in memory :)

lld/MachO/Writer.cpp
565

Are there zerofill sections in segments other than __DATA? __DATA_CONST I guess, but that can't be TLV. (I'm wondering about the plural "segments" here.)

This revision is now accepted and ready to land.Jan 8 2021, 1:38 PM
int3 updated this revision to Diff 315521.Jan 8 2021, 2:26 PM
int3 marked 3 inline comments as done.

rephrase

lld/MachO/InputSection.cpp
50

good point -- rephrased

lld/MachO/Writer.cpp
565

"segments" is plural here because zerofill sections must be at the end of their segments regardless of which segment it is and whether they contain TLV. I'm speaking in generalities :)

(The bss.s test has a test for zerofill at in a non-__DATA segment.)

This revision was automatically updated to reflect the committed changes.