lld/mac mislinks binaries (protoc, chromium, ...) on arm with --deduplicate-literals

[nico]

Bugzilla Link	50791
Version	unspecified
OS	All
Attachments	csd.proto
CC	@gkmhub,@int3,@smeenai

Extended Description

Repro: https://drive.google.com/file/d/1RJe4CRLk3_ULWolz4xZApy6XIJvxMl1R/view?usp=sharing

Link using the response file, then run like so:

./protoc csd.proto --cpp_out .

Then look at csd.pb.h in the output. Line 45 will say

static const ::r_PROTO_NAMESPACE_ID::internal::ParseTableField entries[]

There's a \0 character in the middle of the line, and the line's all wrong.

Remove --deduplicate-literals from that line, link again, and that line looks like

static const ::PROTOBUF_NAMESPACE_ID::internal::ParseTableField entries[]

like it's supposed to do.

[nico]

assigned to @int3

[nico]

I haven't debugged this, but my guess is that this might be due to the alignment thing.

If so, maybe it'd be good if we did the MinAlign thing for cstring subsections too -- keep track of the __cstring section alignment, compute MinAlign(sectionAlign, stringOffset) for each string, and dedup towards larger alignment.

This will require either some surgery to StringTableBuilder, or moving off of it.

Jez, want to take a look at this?

[int3]

Yup, I'll take a look at it

[int3]

Extending this[1] alignment hack to arm64 fixes the problem, so it's definitely an alignment issue.

https://github.com/llvm/llvm-project/blob/main/lld/MachO/SyntheticSections.cpp#L1180

If so, maybe it'd be good if we did the MinAlign thing for cstring subsections too -- keep track of the __cstring section alignment, compute MinAlign(sectionAlign, stringOffset) for each string, and dedup towards larger alignment.

Yeah, I guess we should do that.

[int3]

Test case
When dedup'ing, it appears that ld64 arbitrarily picks one of the alignment/offset pairs for the final string. At least, it doesn't appear to be following some reasonable ordering...

I've attached the test case that I've been using to examine ld64's behavior.

So we may have some freedom to do whatever's most reasonable here. I'm thinking of sorting the CStringInputSections by highest alignment first, then adding their strings to the final CStringSection in that order while preserving the offset of the added cstring. Duplicates will have their offsets ignore. This scheme should allow us to minimize the zero padding in between strings, since we don't have a way to tell when a non-zero string offset is actually required for proper alignment, vs when it is non-zero just because the string has no alignment requirements.

[nico]

I have no strong preference here, so feel free to ignore, but abstractly I would expect that sorting by alignment has close to no effect on output size. So the least surprising behavior to me would be to keep strings in the order they are in the input and just omit dupes that appear later and slide strings after omitted strings left until the hole is gone (but leave enough of a hole so that the slid string is on MinAlign(sectionAlign, stringInputAlign)).

[int3]

Well the sorting was so that dedup'ing toward the larger alignment would be easier to implement.

However, that scheme doesn't seem right. I am running into segfaults while running LLD-linked cppcheck. Input file order doesn't seem to be the right thing either. I'm a bit stumped as to how ld64 is handling this...

[int3]

I am tempted to extend the 16-byte-alignment-hack to arm64 as well and just leave it that way for now. We should find a proper fix eventually, but this doesn't seem super high-pri.

Will have another look later tonight before deciding.

[int3]

https://drive.google.com/file/d/1vH1Loe_y3AgaXvx1X-pH0Xf2n7ruryqQ/view?usp=sharing

This is the cppcheck binary that demonstrates the issue. Patching https://reviews.llvm.org/D104833, linking the repro, and running the testrunner binary will cause a segfault due to a misaligned string access.

[int3]

https://reviews.llvm.org/D104835