This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Don't support relocations in cstring sections
ClosedPublic

Authored by int3 on Jan 4 2023, 7:54 PM.

Details

Summary

We can technically handle them, but since they shouldn't come up in any
real-world programs (since ld64 dedups strings unconditionally), there's
no reason to support them.

It's a thoroughly untested code path too -- as evidenced by the fact
that the only test this change breaks is one that verifies that we
reject relocations when dedup'ing. There is no test that covers the case
where we handle relocations in cstring sections when dedup is disabled.

Diff Detail

Event Timeline

int3 created this revision.Jan 4 2023, 7:54 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 4 2023, 7:54 PM
int3 requested review of this revision.Jan 4 2023, 7:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 7:54 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
oontvoo accepted this revision.Jan 5 2023, 7:06 AM
oontvoo added a subscriber: oontvoo.

LG

This revision is now accepted and ready to land.Jan 5 2023, 7:06 AM
keith accepted this revision.Jan 5 2023, 8:12 AM
thakis added a subscriber: thakis.Jan 5 2023, 9:52 AM

Could we have a diag that's more helpful for users? This looks like a "so what?" diag 🙂 ("Make sure to deduplicate strings" or what)

int3 added a comment.Jan 5 2023, 10:22 AM

Could we have a diag that's more helpful for users? This looks like a "so what?" diag

The point kind of is that there is no user-level remediation though. I don't expect this to actually happen in the wild, and if it does I'd like to have a closer look at the input files to figure out the right solution

I guess I emit the a "file a bug" message like we do for uncaught exceptions but that seems a bit too much?

thakis accepted this revision.Jan 5 2023, 10:59 AM

"file bug" seems more actionable to me, but fair enough 🙂

This revision was automatically updated to reflect the committed changes.