Page MenuHomePhabricator

[ELF] Add -z dead-reloc-in-nonalloc=<section_glob>=<value>
ClosedPublic

Authored by MaskRay on Jul 6 2020, 3:47 PM.

Details

Summary

... to customize the tombstone value we use for an absolute relocation
referencing a discarded symbol. This can be used as a workaround when
some debug processing tool has trouble with current -1 tombstone value
(https://bugs.chromium.org/p/chromium/issues/detail?id=1102223#c11 )

For example, to get the current built-in rules (not considering the .debug_line special case for ICF):

-z dead-reloc-in-nonalloc='.debug_*=0xffffffffffffffff'
-z dead-reloc-in-nonalloc=.debug_loc=0xfffffffffffffffe
-z dead-reloc-in-nonalloc=.debug_ranges=0xfffffffffffffffe

To get GNU ld (as of binutils 2.35)'s behavior:

-z dead-reloc-in-nonalloc='*=0'
-z dead-reloc-in-nonalloc=.debug_ranges=1

This option has other use cases. For example, if we want to check
whether a non-SHF_ALLOC section, say, .foo has dead relocations.
With this patch, we can run a regular LLD and run another with a special
-z dead-reloc-in-nonalloc=, then compare their output.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 6 2020, 3:47 PM
MaskRay updated this revision to Diff 275868.Jul 6 2020, 5:37 PM
MaskRay retitled this revision from [ELF] Add -z dead-nonalloc-reloc=<section_glob>=<value> to [ELF] Add -z dead-reloc-in-nonalloc=<section_glob>=<value>.
MaskRay edited the summary of this revision. (Show Details)
MaskRay added reviewers: grimar, jhenderson, psmith, ruiu, thakis.

Add more tests

MaskRay edited the summary of this revision. (Show Details)Jul 6 2020, 10:00 PM
MaskRay edited the summary of this revision. (Show Details)
psmith added a comment.Jul 7 2020, 2:08 AM

I think an option makes sense. I have a small reservation about making users order their matches on the command line. We should make it more explicit in the help and the manual, or try and sort the matches using a heuristic order of specificity, for example no wildcards is more specific than wildcards, longer text is more specific than shorter. Other than that I think it looks OK.

lld/ELF/Options.td
127 ↗(On Diff #275868)

I think it will be worth mentioning that the there can be multiple occurrences and that the last takes precedence.
"Resolve a relocation from a matched non-SHF_ALLOC section to a discarded symbol to the specified value. Accepts wildcards, in the event of a section matching more than one instance of this option, the last instance on the command-line takes precedence."

lld/docs/ld.lld.1
628

Similar to the comment above. Although as the man-page we can elaborate a bit.
"Resolve a relocation from a matched non-SHF_ALLOC section to a discarded symbol to the specified value. Accepts wildcards, in the event of a section matching more than one instance of this option, the last instance on the command-line takes precedence. An order of least specific
to most specific match is recommended."

Aside from the inline comment, this looks reasonable, but I haven't looked in detail due to time constraints.

lld/test/ELF/dead-reloc-in-nonalloc.s
33–34

I'd be slightly tempted to swap the two arguments around, to show that it is definitely last, and not most-specific, that wins.

grimar added a comment.Jul 7 2020, 3:04 AM

Probably there are other names rather than "dead-reloc-in-nonalloc" which we might want to consider?

-z dead-noalloc-reloc-val
-z tombstone-reloc
-z resolve-dead-reloc

lld/ELF/Driver.cpp
1074

I'd move it after the first continue. Also, perhaps, errPrefix would be more clear name for this variable.

lld/test/ELF/dead-reloc-in-nonalloc.s
10

Please add a comment saying that 2863311530 == 0xaaaaaaaa.

36

I'd leave a single common comment saying you're going to check all possible invalid cases now.
(to separate these tests from above ones).

lld/test/ELF/debug-dead-reloc.s
27

What about printing other sections content too?
(seems there is no other test showing that when override the tombstone value for a debug section, the other ones
remain unaffected)

Probably it worth to combine CHECK and OVERRIDE:

# CHECK:           Contents of section .debug_loc:
# NOOVERRIDE-NEXT: 0000 feffffff ffffffff feffffff ffffffff
# OVERRIDE-NEXT:   0000 2a000000 00000000 2a000000 00000000
...
MaskRay updated this revision to Diff 276098.Jul 7 2020, 9:15 AM
MaskRay marked 8 inline comments as done.

Address comments

lld/ELF/Options.td
127 ↗(On Diff #275868)

Thanks for the suggestion.

My fault, we don't actually have a help for -z options, so I can only leave the message in ld.lld.1
We probably should figure out a way to print -z options in --help.

I pick -z because nonalloc is an ELF specific concept.

lld/test/ELF/debug-dead-reloc.s
27

I think the value testing more section contents is low. This is just an auxiliary test showing that --gc-sections is similar to ICF.

(Moreover, NOOVERRIDE is longer than CHECK and I would need to re-align CHECK-NEXT: above.)

thakis accepted this revision.Jul 7 2020, 5:17 PM

Thanks! Should the release notes grow an entry mentioning the behavior change and this flag?

This revision is now accepted and ready to land.Jul 7 2020, 5:17 PM

Thanks! Should the release notes grow an entry mentioning the behavior change and this flag?

Good idea. Will send a review afterwards.

Cool. Could you check this in, please?

This revision was automatically updated to reflect the committed changes.

Probably there are other names rather than "dead-reloc-in-nonalloc" which we might want to consider?

-z dead-noalloc-reloc-val
-z tombstone-reloc
-z resolve-dead-reloc

This remained unanswered.

Probably there are other names rather than "dead-reloc-in-nonalloc" which we might want to consider?

-z dead-noalloc-reloc-val
-z tombstone-reloc
-z resolve-dead-reloc

This remained unanswered.

Dead relocations in SHF_ALLOC sections are errors. relocateNonAlloc does not handle them, so -z tombstone-reloc and -z resolve-dead-reloc are excluded.

For -z dead-nonalloc-reloc-val The adjective nonalloc appears to describe reloc, which is not precise. nonalloc describes the section.

I still consider -z dead-reloc-in-nonalloc the best.