This is an archive of the discontinued LLVM Phabricator instance.

Differential D48203

[SanitizerCoverage] Add associated metadata to pc-tables.
ClosedPublic

Authored by morehouse on Jun 14 2018, 5:44 PM.

Details

Reviewers
eugenis
Commits
rG0ea9a90b3d6b: [SanitizerCoverage] Add associated metadata to pc-tables.
rCRT334858: [SanitizerCoverage] Add associated metadata to pc-tables.
rL334858: [SanitizerCoverage] Add associated metadata to pc-tables.
Summary

Using associated metadata rather than llvm.used allows linkers to
perform dead stripping with -fsanitize-coverage=pc-table. Unfortunately
in my local tests, LLD was the only linker that made use of this metadata.

Partially addresses https://bugs.llvm.org/show_bug.cgi?id=34636 and fixes
https://github.com/google/sanitizers/issues/971.

Diff Detail

Repository
rL LLVM

Event Timeline

morehouse created this revision.Jun 14 2018, 5:44 PM
Harbormaster completed remote builds in B19369: Diff 151447.Jun 14 2018, 5:44 PM
Herald added a subscriber: hiraditya. · View Herald TranscriptJun 14 2018, 5:44 PM
morehouse edited the summary of this revision. (Show Details)Jun 14 2018, 5:45 PM
morehouse added a subscriber: Dor1s.Jun 14 2018, 6:10 PM
morehouse updated this revision to Diff 151449.Jun 14 2018, 6:47 PM
  • Require LLD for gc-sections.test
Harbormaster completed remote builds in B19370: Diff 151449.Jun 14 2018, 6:49 PM
Dor1s added a comment.Jun 14 2018, 8:06 PM

Nice, thanks for the quick patch, Matt!

eugenis added a comment.Jun 15 2018, 9:22 AM

Did you try BFD with more than one function? There was a fun bug where the "first" (for some definition of the word) input section of an output section was treated differently from the rest.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
601 ↗(On Diff #151449)

I think pctable needs to be in llvm.compiler.used.
!associated alone will not prevent it being thrown away.

morehouse updated this revision to Diff 151525.Jun 15 2018, 9:46 AM
morehouse marked an inline comment as done.
  • Add pc-table to llvm.compiler.used.
morehouse added a comment.Jun 15 2018, 9:47 AM

Neither BFD (version 2.30) nor gold (version 1.15) do dead stripping on my machine, even with multiple unused functions.

Harbormaster completed remote builds in B19387: Diff 151525.Jun 15 2018, 9:47 AM
eugenis added inline comments.Jun 15 2018, 9:50 AM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

this is unnecessary, llvm.used is strictly stronger than llvm.compiler.used

morehouse added inline comments.Jun 15 2018, 9:53 AM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

I'm not sure I understand... We're appending different things to each list.

eugenis added inline comments.Jun 15 2018, 9:59 AM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

Right. Sorry.
A different question then: why only MachO?

morehouse added inline comments.Jun 15 2018, 10:03 AM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

Kostya mentioned his reasoning in http://lists.llvm.org/pipermail/llvm-dev/2017-September/117315.html.

eugenis added inline comments.Jun 15 2018, 10:30 AM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

I don't understand that thread :(
What I know is, without compiler.used, neither comdat nor !associated nor a named section would save pctable from the optimizer. Try passing this through opt -O3:

$a = comdat any
@a = global i32 0, align 4, comdat($a)
@b = internal global i32 0, align 4, section "abc", !associated !0, comdat($a)
; @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (i32* @b to i8*)]
!0 = !{i32 *@a}

morehouse added inline comments.Jun 15 2018, 12:40 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

clang++ -fsanitize=fuzzer -O3 -S -emit-llvm -o- shows the pc-tables still there. But if I run the output of that through opt -O3, the pc-tables get stripped.

So maybe running directly through opt performs some stripping that otherwise wouldn't be done?

eugenis added inline comments.Jun 15 2018, 12:41 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

Most optimizations are done before fuzzer instrumentation.
Try with MSan, it adds a few more at the end.

morehouse added inline comments.Jun 15 2018, 1:00 PM
llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp
409 ↗(On Diff #151525)

With -fsanitize=fuzzer,memory -O3, the pc-tables are still there.

But either way, I don't think it would hurt to add pc-tables to llvm.compiler.used on all platforms.

morehouse updated this revision to Diff 151547.Jun 15 2018, 1:04 PM
  • Add pc-table to llvm.compiler.used on all platforms.
Harbormaster completed remote builds in B19390: Diff 151547.Jun 15 2018, 1:04 PM
morehouse marked an inline comment as done.Jun 15 2018, 1:05 PM
eugenis accepted this revision.Jun 15 2018, 1:10 PM
This revision is now accepted and ready to land.Jun 15 2018, 1:10 PM
Closed by commit rL334858: [SanitizerCoverage] Add associated metadata to pc-tables. (authored by morehouse). · Explain WhyJun 15 2018, 1:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: delcypher. · View Herald TranscriptJun 15 2018, 1:17 PM

Revision Contents

PathSize
compiler-rt/
trunk/
test/
fuzzer/
6 lines
llvm/
trunk/
lib/
Transforms/
Instrumentation/
6 lines

Diff 151549

compiler-rt/trunk/test/fuzzer/gc-sections.test

REQUIRES: linux REQUIRES: linux, lld-available
No gc-sections: No gc-sections:
RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t
RUN: nm %t | grep UnusedFunctionShouldBeRemovedByLinker | count 1 RUN: nm %t | grep UnusedFunctionShouldBeRemovedByLinker | count 1
With gc-sections. Currently, we can't remove unused code. With gc-sections. Currently, we can't remove unused code.
DISABLED: %cpp_compiler %S/GcSectionsTest.cpp -o %t -ffunction-sections -Wl,-gc-sections RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t -ffunction-sections -Wl,-gc-sections -fuse-ld=lld
DISABLED: nm %t | grep UnusedFunctionShouldBeRemovedByLinker | count 1 RUN: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker
With gc sections, with trace-pc. Unused code is removed. With gc sections, with trace-pc. Unused code is removed.
RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t -fsanitize-coverage=0 -fsanitize-coverage=trace-pc -ffunction-sections -Wl,-gc-sections RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t -fsanitize-coverage=0 -fsanitize-coverage=trace-pc -ffunction-sections -Wl,-gc-sections
RUN: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker RUN: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

Show First 20 Lines • Show All 237 Lines • ▼ Show 20 Linesprivate:
Triple TargetTriple; Triple TargetTriple;
LLVMContext *C; LLVMContext *C;
const DataLayout *DL; const DataLayout *DL;
GlobalVariable *FunctionGuardArray; // for trace-pc-guard. GlobalVariable *FunctionGuardArray; // for trace-pc-guard.
GlobalVariable *Function8bitCounterArray; // for inline-8bit-counters. GlobalVariable *Function8bitCounterArray; // for inline-8bit-counters.
GlobalVariable *FunctionPCsArray; // for pc-table. GlobalVariable *FunctionPCsArray; // for pc-table.
SmallVector<GlobalValue *, 20> GlobalsToAppendToUsed; SmallVector<GlobalValue *, 20> GlobalsToAppendToUsed;
SmallVector<GlobalValue *, 20> GlobalsToAppendToCompilerUsed;
SanitizerCoverageOptions Options; SanitizerCoverageOptions Options;
}; };
} // namespace } // namespace
std::pair<GlobalVariable *, GlobalVariable *> std::pair<GlobalVariable *, GlobalVariable *>
SanitizerCoverageModule::CreateSecStartEnd(Module &M, const char *Section, SanitizerCoverageModule::CreateSecStartEnd(Module &M, const char *Section,
▲ Show 20 Lines • Show All 146 Lines • ▼ Show 20 Lines if (Ctor && Options.PCTable) {
IRBCtor.CreateCall(InitFunction, IRBCtor.CreateCall(InitFunction,
{IRB.CreatePointerCast(SecStartEnd.first, IntptrPtrTy), {IRB.CreatePointerCast(SecStartEnd.first, IntptrPtrTy),
IRB.CreatePointerCast(SecStartEnd.second, IntptrPtrTy)}); IRB.CreatePointerCast(SecStartEnd.second, IntptrPtrTy)});
} }
// We don't reference these arrays directly in any of our runtime functions, // We don't reference these arrays directly in any of our runtime functions,
// so we need to prevent them from being dead stripped. // so we need to prevent them from being dead stripped.
if (TargetTriple.isOSBinFormatMachO()) if (TargetTriple.isOSBinFormatMachO())
appendToUsed(M, GlobalsToAppendToUsed); appendToUsed(M, GlobalsToAppendToUsed);
appendToCompilerUsed(M, GlobalsToAppendToCompilerUsed);
return true; return true;
} }
// True if block has successors and it dominates all of them. // True if block has successors and it dominates all of them.
static bool isFullDominator(const BasicBlock *BB, const DominatorTree *DT) { static bool isFullDominator(const BasicBlock *BB, const DominatorTree *DT) {
if (succ_begin(BB) == succ_end(BB)) if (succ_begin(BB) == succ_end(BB))
return false; return false;
▲ Show 20 Lines • Show All 177 Lines • ▼ Show 20 Linesvoid SanitizerCoverageModule::CreateFunctionLocalArrays(
} }
if (Options.Inline8bitCounters) { if (Options.Inline8bitCounters) {
Function8bitCounterArray = CreateFunctionLocalArrayInSection( Function8bitCounterArray = CreateFunctionLocalArrayInSection(
AllBlocks.size(), F, Int8Ty, SanCovCountersSectionName); AllBlocks.size(), F, Int8Ty, SanCovCountersSectionName);
GlobalsToAppendToUsed.push_back(Function8bitCounterArray); GlobalsToAppendToUsed.push_back(Function8bitCounterArray);
} }
if (Options.PCTable) { if (Options.PCTable) {
FunctionPCsArray = CreatePCArray(F, AllBlocks); FunctionPCsArray = CreatePCArray(F, AllBlocks);
GlobalsToAppendToUsed.push_back(FunctionPCsArray); GlobalsToAppendToCompilerUsed.push_back(FunctionPCsArray);
MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
FunctionPCsArray->addMetadata(LLVMContext::MD_associated, *MD);
} }
} }
bool SanitizerCoverageModule::InjectCoverage(Function &F, bool SanitizerCoverageModule::InjectCoverage(Function &F,
ArrayRef<BasicBlock *> AllBlocks, ArrayRef<BasicBlock *> AllBlocks,
bool IsLeafFunc) { bool IsLeafFunc) {
if (AllBlocks.empty()) return false; if (AllBlocks.empty()) return false;
CreateFunctionLocalArrays(F, AllBlocks); CreateFunctionLocalArrays(F, AllBlocks);
▲ Show 20 Lines • Show All 231 Lines • Show Last 20 Lines