[gcov] Add nosanitize metadata to memory access instructions inserted by emitProfileNotes()

[Enna1]

Summary

This patch adds nosantize metadata to memory access instructions inserted by gcov emitProfileNotes(), making sanitizers skip these instructions when gcov and sanitizer are used together.

[nickdesaulniers]

Thanks for the patch! Why do we want to skip sanitizing these loads and stores?

Also, gocv is misspelled in the commit description, please fix that up.

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

150–152

This looks the same as SanitizerMetadata::disableSanitizerForInstruction and ModuleSanitizerCoverage::SetNoSanitizeMetadata. Rather than define a third copy, why not declare it as a new method on Instruction and define it in llvm/lib/IR/Metadata.cpp, then use it in all three places?

925–936

Please use Instruction * rather than auto.
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

[Enna1]

address review comments

[Enna1]

Thanks for you review!

Why do we want to skip sanitizing these loads and stores?

When use asan and gcov together, asan will instrument the loads and stores inserted by gcov which increases the code size a lot.
As the loads and stores inserted by gcov are unlikey to have bugs, so I add nosanitize metadata to these loads and stores like we do in SanitizerCoverage.

Also, gocv is misspelled in the commit description, please fix that up.

Thanks for pointing this.

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

150–152

Send https://reviews.llvm.org/D150632 which adds Instruction::setNoSanitizeMetadata() and use it in SanitizerMetadata and SanitizerCoverage as suggested.

[nickdesaulniers]

In D150460#4344751, @Enna1 wrote:

Why do we want to skip sanitizing these loads and stores?

When use asan and gcov together, asan will instrument the loads and stores inserted by gcov which increases the code size a lot.
As the loads and stores inserted by gcov are unlikey to have bugs, so I add nosanitize metadata to these loads and stores like we do in SanitizerCoverage.

Hmm...that I'm less comfortable with approving. Personally, I feel that coverage and sanitizers don't mix well (perhaps due to such code size increases). If you want low overhead coverage, disable your sanitizers! Perhaps this makes more sense in the context of HWASAN where you might consider enabling a sanitizer always, even in production builds?

I guess I'm worried about __llvm_gcov_ctr becoming some kind of vector for attack.

Though, I guess you wouldn't ship coverage enabled in production.

llvm/test/Transforms/GCOVProfiling/nosanitize.ll

2

Is this run line necessary?

4

please add a comment about the intent of this test; something along the lines of "the intent of this test is to verify that the loads, stores, and atomicrmw adds for gcov are marked nosanitize.

[Enna1]

add comment about the intent of the test

[Enna1]

Though, I guess you wouldn't ship coverage enabled in production.

Sure.

I guess I'm worried about __llvm_gcov_ctr becoming some kind of vector for attack.

__llvm_gcov_ctr global variable is not instrumented by asan, see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1184

Hmm...that I'm less comfortable with approving.

It's ok, if so, I can abandon this patch :)
Or maybe we can add an option gcov-nosanitize?

llvm/test/Transforms/GCOVProfiling/nosanitize.ll

2

Yes, it is for don't writing a.gcno in CWD. See other GCOVProfiling testcase (e.g. atomic-counter.ll, kcfi.ll)

[MaskRay]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

924–925

Define Instruction *Inst outside before if. Call Inst->setNoSanitizemetadata() just once after if.

Add a comment // Disable sanitizers to decrease size bloat. We don't expect sanitizers to catch interesting issues.

llvm/test/Transforms/GCOVProfiling/nosanitize.ll

3

; RUN: rm -rf %t && mkdir %t && cd %t clean up the directory which may be clobbered.

[MaskRay]

In D150460#4348564, @Enna1 wrote:

Though, I guess you wouldn't ship coverage enabled in production.

Sure.

I guess I'm worried about __llvm_gcov_ctr becoming some kind of vector for attack.

__llvm_gcov_ctr global variable is not instrumented by asan, see https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L1184

Hmm...that I'm less comfortable with approving.

It's ok, if so, I can abandon this patch :)
Or maybe we can add an option gcov-nosanitize?

I think this patch is fine. __llvm_gcov_ctr is an internal variable and accessing doesn't use indirection, so no risk of hijection.
I don't recommend mixing gcov and sanitizers (e.g. --coverage -fsanitize=hwaddress), but if a user does this, disabling instrumentation for __llvm_gcov_ctr provides some size benefit.

[nickdesaulniers]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

924–925

It's either 1 or 2 instructions that need the metadata set.

The comment SGTM though that this is intentional/deliberate and not a bug.

[Enna1]

address review comments

[Enna1]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

924–925

Define Instruction *Inst outside before if. Call Inst->setNoSanitizemetadata() just once after if.

There are 2 instructions in else block, so I think can not call Inst->setNoSanitizemetadata() just once after if.

[MaskRay]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

924–925

My idea is the factor out one instruction to apply Inst->setNoSanitizemetadata(). The else branch has one more instruction.

[Enna1]

Update from MaskRay's comments.

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

private:
   insertCounterWriteout(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);
   Function *insertReset(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);
 
   bool AddFlushBeforeForkAndExec();
 
   enum class GCovFileType { GCNO, GCDA };
   std::string mangleName(const DICompileUnit *CU, GCovFileType FileType);
 
   GCOVOptions Options;
   support::endianness Endian;
   raw_ostream *os;
 
   // Checksum, produced by hash of EdgeDestinations
   SmallVector<uint32_t, 4> FileChecksums;
 
   Module *M = nullptr;
   std::function<const TargetLibraryInfo &(Function &F)> GetTLI;
   LLVMContext *Ctx = nullptr;
   SmallVector<std::unique_ptr<GCOVFunction>, 16> Funcs;
for (auto &F : M->functions()) {
             Constant::getNullValue(CounterTy), "__llvm_gcov_ctr");
         CountersBySP.emplace_back(Counters, SP);
 
         for (size_t I : llvm::seq<size_t>(0, Measured)) {
           const Edge &E = *MST.AllEdges[I];
           IRBuilder<> Builder(E.Place, E.Place->getFirstInsertionPt());
           Value *V = Builder.CreateConstInBoundsGEP2_64(
               Counters->getValueType(), Counters, 0, I);
+          // Disable sanitizers to decrease size bloat. We don't expect
+          // sanitizers to catch interesting issues.
           if (Options.Atomic) {
-            Builder.CreateAtomicRMW(AtomicRMWInst::Add, V, Builder.getInt64(1),
-                                    MaybeAlign(), AtomicOrdering::Monotonic);
+            AtomicRMWInst *RMW = Builder.CreateAtomicRMW(
+                AtomicRMWInst::Add, V, Builder.getInt64(1), MaybeAlign(),
+                AtomicOrdering::Monotonic);
+            RMW->setNoSanitizeMetadata();
           } else {
-            Value *Count =
+            LoadInst *OldCount =
                 Builder.CreateLoad(Builder.getInt64Ty(), V, "gcov_ctr");
-            Count = Builder.CreateAdd(Count, Builder.getInt64(1));
-            Builder.CreateStore(Count, V);
+            Value *NewCount = Builder.CreateAdd(OldCount, Builder.getInt64(1));
+            StoreInst *SI = Builder.CreateStore(NewCount, V);
+            OldCount->setNoSanitizeMetadata();
+            SI->setNoSanitizeMetadata();
           }
         }
       }
     }
 
     char Tmp[4];
     JamCRC JC;
     JC.update(EdgeDestinations);
     uint32_t Stamp = JC.getCRC();

llvm/test/Transforms/GCOVProfiling/nosanitize.ll

+;; Ensure that the loads, stores, and atomicrmw adds for gcov have nosanitize metadata.
+; RUN: rm -rf %t && mkdir %t && cd %t
+; RUN: opt < %s -S -passes=insert-gcov-profiling | FileCheck %s
+; RUN: opt < %s -S -passes=insert-gcov-profiling -gcov-atomic-counter | FileCheck %s --check-prefixes=CHECK-ATOMIC-COUNTER
+
+; CHECK-LABEL: void @empty()
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    %gcov_ctr = load i64, ptr @__llvm_gcov_ctr, align 4, !dbg [[DBG:![0-9]+]], !nosanitize [[NOSANITIZE:![0-9]+]]
+; CHECK-NEXT:    %0 = add i64 %gcov_ctr, 1, !dbg [[DBG]]
+; CHECK-NEXT:    store i64 %0, ptr @__llvm_gcov_ctr, align 4, !dbg [[DBG]], !nosanitize [[NOSANITIZE]]
+; CHECK-NEXT:    ret void, !dbg [[DBG]]
+
+; CHECK-ATOMIC-COUNTER-LABEL: void @empty()
+; CHECK-ATOMIC-COUNTER-NEXT:  entry:
+; CHECK-ATOMIC-COUNTER-NEXT:    %0 = atomicrmw add ptr @__llvm_gcov_ctr, i64 1 monotonic, align 8, !dbg [[DBG:![0-9]+]], !nosanitize [[NOSANITIZE:![0-9]+]]
+; CHECK-ATOMIC-COUNTER-NEXT:    ret void, !dbg [[DBG]]
+
+define dso_local void @empty() !dbg !5 {
+entry:
+  ret void, !dbg !8
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "a.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 5}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "empty", scope: !1, file: !1, line: 1, type: !6, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null}
+!8 = !DILocation(line: 2, column: 1, scope: !5)