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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D150460#4344751 <https://reviews.llvm.org/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.



================
Comment at: llvm/test/Transforms/GCOVProfiling/nosanitize.ll:1
+; RUN: mkdir -p %t && cd %t
+; RUN: opt < %s -S -passes=insert-gcov-profiling | FileCheck %s
----------------
Is this run line necessary?


================
Comment at: llvm/test/Transforms/GCOVProfiling/nosanitize.ll:3
+; 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
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150460/new/

https://reviews.llvm.org/D150460