Page MenuHomePhabricator

[SanitizerCoverage] Use different module ctor names for trace-pc-guard and inline-8bit-counters
ClosedPublic

Authored by MaskRay on May 3 2019, 7:41 AM.

Details

Summary

Fixes the main issue in PR41693

When both modes are used, two functions are created:
sancov.module_ctor, sancov.module_ctor.$LastUnique, where
$LastUnique is the current LastUnique counter that may be different in
another module.

sancov.module_ctor.$LastUnique belongs to the comdat group of the same
name (due to the non-null third field of the ctor in llvm.global_ctors).

COMDAT group section [    9] `.group' [sancov.module_ctor] contains 6 sections:
   [Index]    Name
   [   10]   .text.sancov.module_ctor
   [   11]   .rela.text.sancov.module_ctor
   [   12]   .text.sancov.module_ctor.6
   [   13]   .rela.text.sancov.module_ctor.6
   [   23]   .init_array.2
   [   24]   .rela.init_array.2

# 2 problems:
# 1) If sancov.module_ctor in this module is discarded, this group
# has a relocation to a discarded section. ld.bfd and gold will
# error. (Another issue: it is silently accepted by lld)
# 2) The comdat group has an unstable name that may be different in
# another translation unit. Even if the linker allows the dangling relocation
# (with --noinhibit-exec), there will be many undesired .init_array entries
COMDAT group section [   25] `.group' [sancov.module_ctor.6] contains 2 sections:
   [Index]    Name
   [   26]   .init_array.2
   [   27]   .rela.init_array.2

By using different module ctor names, the associated comdat group names
will also be different and thus stable across modules.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.May 3 2019, 7:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 7:41 AM
MaskRay updated this revision to Diff 198122.May 3 2019, 7:01 PM
MaskRay edited the summary of this revision. (Show Details)

Add detailed description

This works for my case that led to me to file https://bugs.llvm.org/show_bug.cgi?id=41693 as a linker issue.

Can you explain how this occurred a bit more and how this is solving the issue?

The second group (which has a distinct group name) references sections in the first group. Why does using two different unsuffixed names cause those new sections to be placed in the correct groups?

Previously the same local symbol name was being used for both things in an uncoordinated fashion. The logic in places like ValueSymbolTable::createValueName leads to the second one getting renamed with some ".digits" suffix to make them unique.
Then the code that emits the references between the init_array and the actual function and sets the COMDAT group name assumed it could use the original name throughout, but in some places this was replaced with the suffixed name and in some places it was not, leading to the incorrect references.

phosek accepted this revision.May 6 2019, 3:58 PM

LGTM but you may want to wait for @kcc @morehouse @vitalybuka to take a look as well since they're the owners.

This revision is now accepted and ready to land.May 6 2019, 3:58 PM
morehouse accepted this revision.May 6 2019, 5:28 PM
morehouse added inline comments.
lib/Transforms/Instrumentation/SanitizerCoverage.cpp
289 ↗(On Diff #198122)

Can we add an assert that the name of CtorFunc matches CtorName?

test/Instrumentation/SanitizerCoverage/trace-pc-guard-inline-8bit-counters.ll
7 ↗(On Diff #198122)

Should we also CHECK-NOT for the suffix?

MaskRay updated this revision to Diff 198385.May 6 2019, 6:18 PM
MaskRay marked 3 inline comments as done.

Apply morehouse's suggestions

test/Instrumentation/SanitizerCoverage/trace-pc-guard-inline-8bit-counters.ll
7 ↗(On Diff #198122)

The .2 suffix is not predictable (well predictable for this small case as we know there is no other duplicate names but probably not good to check the negative pattern).

I'll change it to:

- ; CHECK: define internal void @sancov.module_ctor_trace_pc_guard() comdat
- ; CHECK: define internal void @sancov.module_ctor_8bit_counters() comdat
+ ; CHECK: define internal void @sancov.module_ctor_trace_pc_guard() comdat {
+ ; CHECK: define internal void @sancov.module_ctor_8bit_counters() comdat {

This ensure the comdat name doesn't take the form of comdat (sancov.module_ctor.2) that can cause us trouble.

MaskRay updated this revision to Diff 198386.May 6 2019, 6:35 PM
MaskRay edited the summary of this revision. (Show Details)

Update the description to detail the 2 problems without this patch

This revision was automatically updated to reflect the committed changes.