This is an archive of the discontinued LLVM Phabricator instance.

[SanitizerCoverage] Add associated metadata to PC guards.
ClosedPublic

Authored by morehouse on Aug 8 2018, 5:31 PM.

Details

Summary

Without this metadata LLD strips unused PC table entries
but won't strip unused guards. This metadata also seems
to influence the linker to change the ordering in the PC
guard section to match that of the PC table section.

The libFuzzer runtime library depends on the ordering
of the PC table and PC guard sections being the same. This
is not generally guaranteed, so we may need to redesign
PC tables/guards/counters in the future.

Diff Detail

Event Timeline

morehouse created this revision.Aug 8 2018, 5:31 PM
morehouse edited the summary of this revision. (Show Details)Aug 8 2018, 5:49 PM

I don't understand what's going on.
!associated metadata does not do anything with link order. It prevents linker from discarding a global in some cases, that's all.
Are you relying on the fact that the number of guards is the same as the number of functions? I don't think that's a good idea, because linker GC is not guaranteed to be 100% perfect, and it can also be disabled (or rather not enabled).

compiler-rt/lib/fuzzer/FuzzerTracePC.cpp
230 ↗(On Diff #159835)

Why 100?

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
406 ↗(On Diff #159835)

You are removing MachO-specific code. But !associated is ELF-specific. It has no effect on MachO. What's up with that?

libFuzzer relies on there being the same number of guards as PC table entries, and assumes they are in the same order: https://github.com/llvm-mirror/compiler-rt/blob/dafd5b461e1808fce4a76da13b81065968eff6aa/lib/fuzzer/FuzzerTracePC.cpp#L220

This assumption held until I added associated metadata to PC tables a while back. After that the number of guards still match, but the ordering doesn't. I don't fully understand what the linker is doing either, but adding the metadata to the guards causes the order to match again.

morehouse added inline comments.Aug 9 2018, 4:21 PM
compiler-rt/lib/fuzzer/FuzzerTracePC.cpp
230 ↗(On Diff #159835)

Nothing special about the number except that it's over twice the max distance I've observed between __builtin_return_address(0) from the PC guard callback and the block start address from the PC table.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
406 ↗(On Diff #159835)

You're right, I will add this back.

There are more instances of !associated metadata used in this file on targets other than ELF. I don't know how that works, they should not have any effect, and AFAIK in MachO it means that the counters can be freely discarded.

As I understood, the runtime library relies on the fact that when the linker constructs sections for pcguards and for counters, it does that in the same order. This is not guaranteed.

  • Undo MachO changes.
  • Replace test with addition to gc-sections.test.
morehouse edited the summary of this revision. (Show Details)Aug 10 2018, 10:05 AM
morehouse added a subscriber: kcc.Aug 10 2018, 10:09 AM

As I understood, the runtime library relies on the fact that when the linker constructs sections for pcguards and for counters, it does that in the same order. This is not guaranteed.

Removed test for this behavior and instead added a test analogous to what we do for -fsanitize-coverage=inline-8bit-counters,pc-table. When @kcc is back, maybe we should discuss changing SanCov to not rely on the same ordering.

Ping. Can we submit this patch at least for consistency with PC tables and inline counters?

eugenis accepted this revision.Aug 14 2018, 2:49 PM

OK. But please mention the fact that runtime library relies on the section entry order in the patch description.

This revision is now accepted and ready to land.Aug 14 2018, 2:49 PM
morehouse edited the summary of this revision. (Show Details)Aug 14 2018, 3:02 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald TranscriptAug 14 2018, 3:05 PM