[SanitizerCoverage] Add associated metadata to PC guards.

[morehouse]

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.

[eugenis]

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

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

[morehouse]

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]

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

You're right, I will add this back.

[eugenis]

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.

[morehouse]

• Undo MachO changes.
• Replace test with addition to gc-sections.test.

[morehouse]

In D50483#1194694, @eugenis wrote:

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.

[morehouse]

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

[eugenis]

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

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

 REQUIRES: linux, lld-available
 
 No gc-sections:
 RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t
 RUN: nm %t | grep UnusedFunctionShouldBeRemovedByLinker | count 1
 
 With gc-sections. Currently, we can't remove unused code except with LLD.
 RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t -fuse-ld=lld -ffunction-sections -Wl,-gc-sections
 RUN: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker
 RUN: %run %t -runs=0 2>&1 | FileCheck %s
-CHECK-NOT: ERROR: The size of coverage PC tables does not match
 
 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: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker
+
+RUN: %cpp_compiler %S/GcSectionsTest.cpp -o %t -fsanitize-coverage=0 -fsanitize-coverage=trace-pc-guard,pc-table -fuse-ld=lld -ffunction-sections -Wl,-gc-sections
+RUN: nm %t | not grep UnusedFunctionShouldBeRemovedByLinker
+RUN: %run %t -runs=0 2>&1 | FileCheck %s
+
+CHECK-NOT: ERROR: The size of coverage PC tables does not match

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

Function *InitFunction = declareSanitizerInitFunction(
         M, SanCovPCsInitName, {IntptrPtrTy, IntptrPtrTy});
     IRBuilder<> IRBCtor(Ctor->getEntryBlock().getTerminator());
     IRBCtor.CreateCall(InitFunction,
                        {IRB.CreatePointerCast(SecStartEnd.first, IntptrPtrTy),
                         IRB.CreatePointerCast(SecStartEnd.second, IntptrPtrTy)});
   }
   // We don't reference these arrays directly in any of our runtime functions,
   // so we need to prevent them from being dead stripped.
   if (TargetTriple.isOSBinFormatMachO())
     appendToUsed(M, GlobalsToAppendToUsed);
   appendToCompilerUsed(M, GlobalsToAppendToCompilerUsed);
   return true;
 }
 
 // True if block has successors and it dominates all of them.
 static bool isFullDominator(const BasicBlock *BB, const DominatorTree *DT) {
   if (succ_begin(BB) == succ_end(BB))

 }
 
 void SanitizerCoverageModule::CreateFunctionLocalArrays(
     Function &F, ArrayRef<BasicBlock *> AllBlocks) {
   if (Options.TracePCGuard) {
     FunctionGuardArray = CreateFunctionLocalArrayInSection(
         AllBlocks.size(), F, Int32Ty, SanCovGuardsSectionName);
     GlobalsToAppendToUsed.push_back(FunctionGuardArray);
+    GlobalsToAppendToCompilerUsed.push_back(FunctionGuardArray);
+    MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
+    FunctionGuardArray->addMetadata(LLVMContext::MD_associated, *MD);
   }
   if (Options.Inline8bitCounters) {
     Function8bitCounterArray = CreateFunctionLocalArrayInSection(
         AllBlocks.size(), F, Int8Ty, SanCovCountersSectionName);
     GlobalsToAppendToCompilerUsed.push_back(Function8bitCounterArray);
     MDNode *MD = MDNode::get(F.getContext(), ValueAsMetadata::get(&F));
     Function8bitCounterArray->addMetadata(LLVMContext::MD_associated, *MD);
   }