[nsan] Emit calls to optimized functions #98900

chestnykh · 2024-07-15T13:39:03Z

As previously noted in nsan.cpp we can implement
optimized variants of __nsan_copy_values and
__nsan_set_value_unknown if a memory operation
size is known.
Now the instrumentation creates calls to optimized functions if there is 4, 8 or 16-byte memory operation like
memset(X, value, 4/8/16) or memcpy(dst, src, 4/8/16)
nsan.cpp provides definitions of the optimized functions.

llvmbot · 2024-07-15T13:39:34Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Dmitriy Chestnykh (chestnykh)

Changes

As previously noted in nsan.cpp we can implement
optimized variants of __nsan_copy_values and
__nsan_set_value_unknown if a memory operation
size is known.
Now the instrumentation creates calls to optimized functions if there is 4, 8 or 16-byte memory operation like
memset(X, value, 4/8/16) or memcpy(dst, src, 4/8/16)
nsan.cpp provides definition of the optimized functions.

Full diff: https://github.com/llvm/llvm-project/pull/98900.diff

3 Files Affected:

(modified) compiler-rt/lib/nsan/nsan.cpp (+22-4)
(modified) llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp (+71-16)
(modified) llvm/test/Instrumentation/NumericalStabilitySanitizer/memory.ll (+86-3)

diff --git a/compiler-rt/lib/nsan/nsan.cpp b/compiler-rt/lib/nsan/nsan.cpp
index e5a9b7a036e0e..e60dc4f5c3a70 100644
--- a/compiler-rt/lib/nsan/nsan.cpp
+++ b/compiler-rt/lib/nsan/nsan.cpp
@@ -54,8 +54,6 @@ using namespace __nsan;
 constexpr int kMaxVectorWidth = 8;
 
 // When copying application memory, we also copy its shadow and shadow type.
-// FIXME: We could provide fixed-size versions that would nicely
-// vectorize for known sizes.
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE void
 __nsan_copy_values(const u8 *daddr, const u8 *saddr, uptr size) {
   internal_memmove((void *)GetShadowTypeAddrFor(daddr),
@@ -64,13 +62,33 @@ __nsan_copy_values(const u8 *daddr, const u8 *saddr, uptr size) {
                    size * kShadowScale);
 }
 
-// FIXME: We could provide fixed-size versions that would nicely
-// vectorize for known sizes.
+#define NSAN_COPY_VALUES_N(N)                                                  \
+  extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __nsan_copy_##N(               \
+      const u8 *daddr, const u8 *saddr) {                                      \
+    __builtin_memmove((void *)GetShadowTypeAddrFor(daddr),                     \
+                      GetShadowTypeAddrFor(saddr), N);                         \
+    __builtin_memmove((void *)GetShadowAddrFor(daddr),                         \
+                      GetShadowAddrFor(saddr), N *kShadowScale);               \
+  }
+
+NSAN_COPY_VALUES_N(4);
+NSAN_COPY_VALUES_N(8);
+NSAN_COPY_VALUES_N(16);
+
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE void
 __nsan_set_value_unknown(const u8 *addr, uptr size) {
   internal_memset((void *)GetShadowTypeAddrFor(addr), 0, size);
 }
 
+#define NSAN_SET_VALUE_UNKNOWN_N(N)                                            \
+  extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __nsan_set_value_unknown_##N(  \
+      const u8 *daddr) {                                                       \
+    __builtin_memset((void *)GetShadowTypeAddrFor(daddr), 0, N);               \
+  }
+
+NSAN_SET_VALUE_UNKNOWN_N(4);
+NSAN_SET_VALUE_UNKNOWN_N(8);
+NSAN_SET_VALUE_UNKNOWN_N(16);
 
 const char *FTInfo<float>::kCppTypeName = "float";
 const char *FTInfo<double>::kCppTypeName = "double";
diff --git a/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp
index 99b1c779f3167..607b4384b5136 100644
--- a/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp
@@ -550,12 +550,15 @@ class NumericalStabilitySanitizer {
   LLVMContext &Context;
   MappingConfig Config;
   IntegerType *IntptrTy = nullptr;
+
+  // TODO: Use std::array instead?
   FunctionCallee NsanGetShadowPtrForStore[FTValueType::kNumValueTypes] = {};
   FunctionCallee NsanGetShadowPtrForLoad[FTValueType::kNumValueTypes] = {};
   FunctionCallee NsanCheckValue[FTValueType::kNumValueTypes] = {};
   FunctionCallee NsanFCmpFail[FTValueType::kNumValueTypes] = {};
-  FunctionCallee NsanCopyValues;
-  FunctionCallee NsanSetValueUnknown;
+
+  std::array<FunctionCallee, 4> NsanCopyFunction;
+  std::array<FunctionCallee, 4> NsanSetValueUnknownFunction;
   FunctionCallee NsanGetRawShadowTypePtr;
   FunctionCallee NsanGetRawShadowPtr;
   GlobalValue *NsanShadowRetTag = nullptr;
@@ -634,10 +637,27 @@ NumericalStabilitySanitizer::NumericalStabilitySanitizer(Module &M)
         Attr, VoidTy, VTTy, VTTy, ShadowTy, ShadowTy, Int32Ty, Int1Ty, Int1Ty);
   }
 
-  NsanCopyValues = M.getOrInsertFunction("__nsan_copy_values", Attr, VoidTy,
-                                         PtrTy, PtrTy, IntptrTy);
-  NsanSetValueUnknown = M.getOrInsertFunction("__nsan_set_value_unknown", Attr,
-                                              VoidTy, PtrTy, IntptrTy);
+  NsanCopyFunction[0] =
+      M.getOrInsertFunction("__nsan_copy_4", Attr, VoidTy, PtrTy, PtrTy);
+  NsanCopyFunction[1] =
+      M.getOrInsertFunction("__nsan_copy_8", Attr, VoidTy, PtrTy, PtrTy);
+  NsanCopyFunction[2] =
+      M.getOrInsertFunction("__nsan_copy_16", Attr, VoidTy, PtrTy, PtrTy);
+
+  NsanCopyFunction[3] = M.getOrInsertFunction("__nsan_copy_values", Attr,
+                                              VoidTy, PtrTy, PtrTy, IntptrTy);
+
+  NsanSetValueUnknownFunction[0] =
+      M.getOrInsertFunction("__nsan_set_value_unknown_4", Attr, VoidTy, PtrTy);
+
+  NsanSetValueUnknownFunction[1] =
+      M.getOrInsertFunction("__nsan_set_value_unknown_8", Attr, VoidTy, PtrTy);
+
+  NsanSetValueUnknownFunction[2] =
+      M.getOrInsertFunction("__nsan_set_value_unknown_16", Attr, VoidTy, PtrTy);
+
+  NsanSetValueUnknownFunction[3] = M.getOrInsertFunction(
+      "__nsan_set_value_unknown", Attr, VoidTy, PtrTy, IntptrTy);
 
   // TODO: Add attributes nofree, nosync, readnone, readonly,
   NsanGetRawShadowTypePtr = M.getOrInsertFunction(
@@ -1880,7 +1900,7 @@ void NumericalStabilitySanitizer::propagateNonFTStore(
     }
   }
   // All other stores just reset the shadow value to unknown.
-  Builder.CreateCall(NsanSetValueUnknown, {Dst, ValueSize});
+  Builder.CreateCall(NsanSetValueUnknownFunction.back(), {Dst, ValueSize});
 }
 
 void NumericalStabilitySanitizer::propagateShadowValues(
@@ -2123,21 +2143,56 @@ bool NumericalStabilitySanitizer::sanitizeFunction(
   return !ValueToShadow.empty();
 }
 
+static size_t GetInstrumentationCalleeIdxForMemOp(Value *V,
+                                                  std::size_t MaxIdx) {
+  uint64_t OpSize = 0;
+  if (Constant *C = dyn_cast<Constant>(V)) {
+    auto *CInt = dyn_cast<ConstantInt>(C);
+    if (CInt && CInt->getValue().getBitWidth() <= 64)
+      OpSize = CInt->getValue().getZExtValue();
+  }
+
+  size_t CandidateIdx =
+      OpSize == 4 ? 0 : (OpSize == 8 ? 1 : (OpSize == 16 ? 2 : MaxIdx));
+
+  return CandidateIdx <= MaxIdx ? CandidateIdx : MaxIdx;
+}
+
 // Instrument the memory intrinsics so that they properly modify the shadow
 // memory.
 bool NumericalStabilitySanitizer::instrumentMemIntrinsic(MemIntrinsic *MI) {
   IRBuilder<> Builder(MI);
   if (auto *M = dyn_cast<MemSetInst>(MI)) {
-    Builder.CreateCall(
-        NsanSetValueUnknown,
-        {/*Address=*/M->getArgOperand(0),
-         /*Size=*/Builder.CreateIntCast(M->getArgOperand(2), IntptrTy, false)});
+    Value *OpSizeValue = M->getArgOperand(2);
+    size_t NrSetValueUnknownFns = NsanSetValueUnknownFunction.size();
+    size_t NsanSetValueUnknownFunctionIdx = GetInstrumentationCalleeIdxForMemOp(
+        OpSizeValue, NrSetValueUnknownFns - 1);
+    if (NsanSetValueUnknownFunctionIdx != NrSetValueUnknownFns - 1)
+      Builder.CreateCall(
+          NsanSetValueUnknownFunction[NsanSetValueUnknownFunctionIdx],
+          {/*Address=*/M->getArgOperand(0)});
+    else
+      Builder.CreateCall(NsanSetValueUnknownFunction.back(),
+                         {/*Address=*/M->getArgOperand(0),
+                          /*Size=*/Builder.CreateIntCast(M->getArgOperand(2),
+                                                         IntptrTy, false)});
+
   } else if (auto *M = dyn_cast<MemTransferInst>(MI)) {
-    Builder.CreateCall(
-        NsanCopyValues,
-        {/*Destination=*/M->getArgOperand(0),
-         /*Source=*/M->getArgOperand(1),
-         /*Size=*/Builder.CreateIntCast(M->getArgOperand(2), IntptrTy, false)});
+    Value *OpSizeValue = M->getArgOperand(2);
+    size_t NrCopyFns = NsanCopyFunction.size();
+    size_t NsanCopyFunctionIdx =
+        GetInstrumentationCalleeIdxForMemOp(OpSizeValue, NrCopyFns - 1);
+
+    if (NsanCopyFunctionIdx != NrCopyFns - 1)
+      Builder.CreateCall(NsanCopyFunction[NsanCopyFunctionIdx],
+                         {/*Destination=*/M->getArgOperand(0),
+                          /*Source=*/M->getArgOperand(1)});
+    else
+      Builder.CreateCall(
+          NsanCopyFunction.back(),
+          {/*Destination=*/M->getArgOperand(0),
+           /*Source=*/M->getArgOperand(1),
+           /*Size=*/Builder.CreateIntCast(OpSizeValue, IntptrTy, false)});
   }
   return false;
 }
diff --git a/llvm/test/Instrumentation/NumericalStabilitySanitizer/memory.ll b/llvm/test/Instrumentation/NumericalStabilitySanitizer/memory.ll
index 3fe78d8b19b0a..1f238b88f49a4 100644
--- a/llvm/test/Instrumentation/NumericalStabilitySanitizer/memory.ll
+++ b/llvm/test/Instrumentation/NumericalStabilitySanitizer/memory.ll
@@ -7,10 +7,10 @@ target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f3
 
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1)
 
-define void @call_memcpy_intrinsic(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
-; CHECK-LABEL: @call_memcpy_intrinsic(
+define void @call_memcpy_intrinsic_16bytes(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memcpy_intrinsic_16bytes(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    call void @__nsan_copy_values(ptr [[A:%.*]], ptr [[B:%.*]], i64 16)
+; CHECK-NEXT:    call void @__nsan_copy_16(ptr [[A:%.*]], ptr [[B:%.*]])
 ; CHECK-NEXT:    tail call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], ptr nonnull align 8 dereferenceable(16) [[B]], i64 16, i1 false)
 ; CHECK-NEXT:    ret void
 ;
@@ -19,6 +19,42 @@ entry:
   ret void
 }
 
+define void @call_memcpy_intrinsic_8bytes(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memcpy_intrinsic_8bytes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_copy_8(ptr [[A:%.*]], ptr [[B:%.*]])
+; CHECK-NEXT:    tail call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], ptr nonnull align 8 dereferenceable(16) [[B]], i64 8, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(ptr nonnull align 8 dereferenceable(16) %a, ptr nonnull align 8 dereferenceable(16) %b, i64 8, i1 false)
+  ret void
+}
+
+define void @call_memcpy_intrinsic_4bytes(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memcpy_intrinsic_4bytes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_copy_4(ptr [[A:%.*]], ptr [[B:%.*]])
+; CHECK-NEXT:    tail call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], ptr nonnull align 8 dereferenceable(16) [[B]], i64 4, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(ptr nonnull align 8 dereferenceable(16) %a, ptr nonnull align 8 dereferenceable(16) %b, i64 4, i1 false)
+  ret void
+}
+
+define void @call_memcpy_intrinsic(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memcpy_intrinsic(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_copy_values(ptr [[A:%.*]], ptr [[B:%.*]], i64 15)
+; CHECK-NEXT:    tail call void @llvm.memcpy.p0.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], ptr nonnull align 8 dereferenceable(16) [[B]], i64 15, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(ptr nonnull align 8 dereferenceable(16) %a, ptr nonnull align 8 dereferenceable(16) %b, i64 15, i1 false)
+  ret void
+}
+
 declare dso_local i8* @memcpy(i8*, i8*, i64) local_unnamed_addr
 
 define void @call_memcpy(i8* nonnull align 8 dereferenceable(16) %a, i8* nonnull align 8 dereferenceable(16) %b) sanitize_numerical_stability {
@@ -32,6 +68,53 @@ entry:
   ret void
 }
 
+define void @call_memset_intrinsic_16bytes(i8* nonnull align 8 dereferenceable(16) %a) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memset_intrinsic_16bytes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_set_value_unknown_16(ptr [[A:%.*]])
+; CHECK-NEXT:    tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], i8 0, i64 16, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) %a, i8 0, i64 16, i1 false)
+  ret void
+}
+
+define void @call_memset_intrinsic_8bytes(i8* nonnull align 8 dereferenceable(16) %a) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memset_intrinsic_8bytes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_set_value_unknown_8(ptr [[A:%.*]])
+; CHECK-NEXT:    tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], i8 0, i64 8, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) %a, i8 0, i64 8, i1 false)
+  ret void
+}
+
+define void @call_memset_intrinsic_4bytes(i8* nonnull align 8 dereferenceable(16) %a) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memset_intrinsic_4bytes(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_set_value_unknown_4(ptr [[A:%.*]])
+; CHECK-NEXT:    tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], i8 0, i64 4, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) %a, i8 0, i64 4, i1 false)
+  ret void
+}
+
+define void @call_memset_intrinsic(i8* nonnull align 8 dereferenceable(16) %a) sanitize_numerical_stability {
+; CHECK-LABEL: @call_memset_intrinsic(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @__nsan_set_value_unknown(ptr [[A:%.*]], i64 15)
+; CHECK-NEXT:    tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) [[A]], i8 0, i64 15, i1 false)
+; CHECK-NEXT:    ret void
+;
+entry:
+  tail call void @llvm.memset.p0.i64(ptr nonnull align 8 dereferenceable(16) %a, i8 0, i64 15, i1 false)
+  ret void
+}
 
 define void @transfer_float(float* %dst, float* %src) sanitize_numerical_stability {
 ; CHECK-LABEL: @transfer_float(

chestnykh · 2024-07-15T13:39:38Z

CC: @MaskRay @alexander-shaposhnikov @vitalybuka

chestnykh · 2024-07-16T08:27:05Z

CC: @nikic

alexander-shaposhnikov · 2024-07-16T17:55:38Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

@@ -2123,21 +2143,53 @@ bool NumericalStabilitySanitizer::sanitizeFunction(
  return !ValueToShadow.empty();
 }

+static size_t GetInstrumentationCalleeIdxForMemOp(Value *V,
+                                                  std::size_t MaxIdx) {


for consistency one can drop std::

alexander-shaposhnikov · 2024-07-16T18:05:07Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

-                                         PtrTy, PtrTy, IntptrTy);
-  NsanSetValueUnknown = M.getOrInsertFunction("__nsan_set_value_unknown", Attr,
-                                              VoidTy, PtrTy, IntptrTy);
+  NsanCopyFunction[0] =


Since now we have multiple functions, arrays, and logic in GetInstrumentationCalleeIdxForMemOp, it'd be better to encapsulate them into helper classes, e.g., class NsanCopy and class NsanSetUnknown, to keep the internal details grouped together.

Done. I've tried to do it with one base class which declares some basic virtual methods and a couple of derived class one per function family

alexander-shaposhnikov · 2024-07-16T19:52:03Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

@@ -493,6 +494,103 @@ class ValueToShadowMap {
  DenseMap<Value *, Value *> Map;
 };

+// Base class for handling some details of __nsan_* functions
+class NsanInstrumentationFunction {


The classes share the interface, but they are not used polymorphically, so the base class doesn't help
much / I'd simplify things by removing it.

Done in the latest commit

alexander-shaposhnikov · 2024-07-16T20:19:56Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+  virtual ~NsanInstrumentationFunction() = default;
+};
+
+// Helper class for __nsan_copy_* family


How about simplifying / removing duplication, e.g. something along these lines:

template <size_t N> class NsanMemOp { public: NsanMemOp(Module &M, ArrayRef<StringRef> Sized, StringRef Fallback); // Number of parameters can be extracted from FunctionCallee FunctionCallee getFunctionFor(uint64_t MemOpSize) const; FunctionCallee getFallback() const; private: std::array<FunctionCallee, N> Funcs; }; NsanMemOp<4> NsanCopy; NsanMemOp<4> NsanSetUnknown;

thanks for the tip, removing duplication is really something that would be nice here

Done. I use the second template argument to pass information about number of arguments of the functions represented by the instance. This information is needed by constructor

Or we can just pass this number as constructor argument?
UPD: removed 2nd template argument. now NFallbackArgs is just ctor parameter

chestnykh · 2024-07-17T05:34:16Z

compiler-rt/lib/nsan/nsan.cpp

+    __builtin_memmove((void *)GetShadowTypeAddrFor(daddr),                     \
+                      GetShadowTypeAddrFor(saddr), N);                         \
+    __builtin_memmove((void *)GetShadowAddrFor(daddr),                         \
+                      GetShadowAddrFor(saddr), N *kShadowScale);               \


Here clang-format failed to format the code correctly.
N *kShadowScale means 'pointer of type N' when we have multiplication here.
#99271

chestnykh · 2024-07-17T05:39:23Z

I've rebased and force-pushed because bots couldn't start jobs for this PR
p.s. code formatting fails because of the clang-format issue

github-actions · 2024-07-17T05:42:06Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff dbe308c000f3401cbf6bb55f2b8d606fe091dcfe d94755925710212fc6d2bf8682e7bc59df59acf1 --extensions cpp -- compiler-rt/lib/nsan/nsan.cpp llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

View the diff from clang-format here.

diff --git a/compiler-rt/lib/nsan/nsan.cpp b/compiler-rt/lib/nsan/nsan.cpp
index 568a1b3a65..ad1e64f7f7 100644
--- a/compiler-rt/lib/nsan/nsan.cpp
+++ b/compiler-rt/lib/nsan/nsan.cpp
@@ -68,7 +68,7 @@ __nsan_copy_values(const u8 *daddr, const u8 *saddr, uptr size) {
     __builtin_memmove((void *)GetShadowTypeAddrFor(daddr),                     \
                       GetShadowTypeAddrFor(saddr), N);                         \
     __builtin_memmove((void *)GetShadowAddrFor(daddr),                         \
-                      GetShadowAddrFor(saddr), N * kShadowScale);              \
+                      GetShadowAddrFor(saddr), N *kShadowScale);               \
   }
 
 NSAN_COPY_VALUES_N(4)

alexander-shaposhnikov · 2024-07-17T06:11:23Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+
+  for (size_t i = 0; i < N - 1; ++i) {
+    if (NFallbackArgs == 3)
+      Funcs[i] = M.getOrInsertFunction(Sized[i], Attr, VoidTy, PtrTy, PtrTy);


this can be simplified further - getOrInsertFunction has other overloads that take function type (can be computed once based on the number of parameters)

alexander-shaposhnikov · 2024-07-17T06:14:15Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+
+template <size_t N>
+NsanMemOpFn<N>::NsanMemOpFn(Module &M, ArrayRef<StringRef> Sized,
+                            StringRef Fallback, int NFallbackArgs) {


nit: int NFallBackArgs -> size_t NumArgs

alexander-shaposhnikov · 2024-07-17T06:15:16Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+      NsanCopyFns(M, {"__nsan_copy_4", "__nsan_copy_8", "__nsan_copy_16"},
+                  "__nsan_copy_values", 3),
+      NsanSetUnknownFns(M,
+                        {"__nsan_set_value_unknown_4",


nit: git clang-format --style=llvm

this command tells me that there are no modified files and the style is correct

I've corrected codestyle of previous commits, but these lines stay unchanged

chestnykh · 2024-07-17T06:28:40Z

Sorry, i've pushed wrong commits
UPD: fixed

alexander-shaposhnikov · 2024-07-17T23:19:35Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

@@ -493,6 +493,59 @@ class ValueToShadowMap {
  DenseMap<Value *, Value *> Map;
 };

+// Template parameter is the number of functions
+template <size_t N> class NsanMemOpFn {


What would you say to pushing it even further, e.g. drop the template parameter ?
The number of sized variants is known in the constructor (Sized.size()),
std::array can be replaced with SmallVector (don't think the size of the data structure is a particular concern here).
p.s. If the code in getFunctionFor is specific to a particular value of N, we should add a comment and an assert (or asserts).

Yes, good idea. Template parameter is not really needed anymore here

alexander-shaposhnikov · 2024-07-18T00:50:13Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

-    : DL(M.getDataLayout()), Context(M.getContext()), Config(Context) {
+    : DL(M.getDataLayout()), Context(M.getContext()), Config(Context),
+      NsanCopyFns(M, {"__nsan_copy_4", "__nsan_copy_8", "__nsan_copy_16"},
+                  "__nsan_copy_values", 3),


Need a comment (e.g. 3 /*NumArgs*/)

alexander-shaposhnikov · 2024-07-18T01:04:04Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+  for (size_t i = 0; i < N - 1; ++i)
+    Funcs[i] = M.getOrInsertFunction(Sized[i], SizedFnTy, Attr);
+
+  if (NumArgs == 3)


nit: it'd look a bit more natural with a single if-else:

if (NumArgs == 3) { Funcs.back() = M.getOrInsertFunction(Fallback, Attr, VoidTy, PtrTy, PtrTy, IntptrTy); SizedFnTy = FunctionType::get(VoidTy, {PtrTy, PtrTy}, false); } else if (NumArgs == 2) { Funcs.back() = M.getOrInsertFunction(Fallback, Attr, VoidTy, PtrTy, IntptrTy); SizedFnTy = FunctionType::get(VoidTy, {PtrTy}, false); } else { assert(!"Unexpected value of NumArgs"); }

alexander-shaposhnikov · 2024-07-18T05:59:12Z

To me looks good.
P.S. Do you mind if we hold off on landing this for a couple of days - I wanted to publish a PR with integration tests, this would give us a bit more confidence that things still work. In the meantime, would be great to try to build a ~relatively large program with this PR, although you might encounter other issues (most likely #98143)

alexander-shaposhnikov · 2024-07-18T05:59:55Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+public:
+  NsanMemOpFn(Module &M, ArrayRef<StringRef> Sized, StringRef Fallback,
+              size_t NumArgs);
+  // Number of parameters can be extracted from FunctionCallee


nit: this comment can be dropped.

chestnykh · 2024-07-24T06:34:14Z

Rebase to check this PR on the nsan tests that was landed recently

As previously noted in nsan.cpp we can implement optimized variants of `__nsan_copy_values` and `__nsan_set_value_unknown` if a memory operation size is known. Now the instrumentation creates calls to optimized functions if there is 4, 8 or 16-byte memory operation like `memset(X, value, 4/8/16)` or `memcpy(dst, src, 4/8/16)` nsan.cpp provides definition of the optimized functions.

- Test all calls to `__nsan_copy_*` in one function - Test all calls to `__nsan_set_value_unknown_*` in one function

Add base class for handling details of some `__nsan_*` functions Add derived `NsanCopy` and `NsanSetUnknownClasses` with the appropriate methods

Use one class instead of base and derived classes

clang-format cannot recognize that this is multiplication, not pointer declaration. See llvm#99271

With this approach the constructor of `NsanMemOpFn` can be simplified

chestnykh · 2024-07-24T07:00:23Z

Force pushed to remove unrelated changes added accidentally

alexander-shaposhnikov · 2024-07-24T07:32:38Z

@chestnykh - do you want me to try to merge this PR ?

chestnykh · 2024-07-24T07:34:43Z

@chestnykh - do you want me to try to merge this PR ?

Yes,you can merge it, but lets wait for checks to complete

chestnykh · 2024-07-24T08:20:28Z

Checked locally that make check-nsan suceeds

hctim · 2024-07-24T09:00:21Z

llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp

+        M.getOrInsertFunction(Fallback, Attr, VoidTy, PtrTy, IntptrTy));
+    SizedFnTy = FunctionType::get(VoidTy, {PtrTy}, false);
+  } else {
+    assert(!"Unexpected value of sized functions arguments");


Looks like this breaks under buildbots with -Werror: https://lab.llvm.org/buildbot/#/builders/109/builds/1300

[3128/3995] Building CXX object lib/Transforms/Instrumentation/CMakeFiles/LLVMInstrumentation.dir/NumericalStabilitySanitizer.cpp.o FAILED: lib/Transforms/Instrumentation/CMakeFiles/LLVMInstrumentation.dir/NumericalStabilitySanitizer.cpp.o CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm_build0/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/build_default/lib/Transforms/Instrumentation -I/home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/llvm/lib/Transforms/Instrumentation -I/home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/build_default/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -std=c++17 -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT lib/Transforms/Instrumentation/CMakeFiles/LLVMInstrumentation.dir/NumericalStabilitySanitizer.cpp.o -MF lib/Transforms/Instrumentation/CMakeFiles/LLVMInstrumentation.dir/NumericalStabilitySanitizer.cpp.o.d -o lib/Transforms/Instrumentation/CMakeFiles/LLVMInstrumentation.dir/NumericalStabilitySanitizer.cpp.o -c /home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp /home/buildbots/llvm-external-buildbots/workers/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm-project/llvm/lib/Transforms/Instrumentation/NumericalStabilitySanitizer.cpp:530:13: error: implicit conversion turns string literal into bool: 'const char[46]' to 'bool' [-Werror,-Wstring-conversion] 530 | assert(!"Unexpected value of sized functions arguments"); | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/include/assert.h:87:5: note: expanded from macro 'assert' 87 | ((expr) \ | ^~~~ 1 error generated.

Summary: As previously noted in nsan.cpp we can implement optimized variants of `__nsan_copy_values` and `__nsan_set_value_unknown` if a memory operation size is known. Now the instrumentation creates calls to optimized functions if there is 4, 8 or 16-byte memory operation like `memset(X, value, 4/8/16)` or `memcpy(dst, src, 4/8/16)` nsan.cpp provides definitions of the optimized functions. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250636

llvmbot added compiler-rt compiler-rt:sanitizer llvm:transforms labels Jul 15, 2024

dtcxzyw requested review from alexander-shaposhnikov, MaskRay and vitalybuka July 15, 2024 15:03

alexander-shaposhnikov reviewed Jul 16, 2024

View reviewed changes

chestnykh commented Jul 17, 2024

View reviewed changes

chestnykh force-pushed the nsan-optimized-instrumentation branch from c09dde4 to cbf02ed Compare July 17, 2024 05:38

alexander-shaposhnikov reviewed Jul 17, 2024

View reviewed changes

chestnykh force-pushed the nsan-optimized-instrumentation branch 2 times, most recently from 6b599b8 to 16320d2 Compare July 17, 2024 06:31

vitalybuka requested a review from alexander-shaposhnikov July 17, 2024 22:43

alexander-shaposhnikov reviewed Jul 17, 2024

View reviewed changes

alexander-shaposhnikov reviewed Jul 18, 2024

View reviewed changes

alexander-shaposhnikov approved these changes Jul 18, 2024

View reviewed changes

chestnykh added 18 commits July 24, 2024 09:35

[compiler-rt][nsan] Remove unneeded semicolons

67eefd0

[nsan] Optimize GetInstrumentationCalleeIdxForMemOp

74c08d5

[nsan] Simplify memory.ll test

1d582cf

- Test all calls to `__nsan_copy_*` in one function - Test all calls to `__nsan_set_value_unknown_*` in one function

[nsan] Use size_t instead of std::size_t

397ec05

[nsan] Add helper classes

d523d38

Add base class for handling details of some `__nsan_*` functions Add derived `NsanCopy` and `NsanSetUnknownClasses` with the appropriate methods

[nsan] Remove unneeded newline

ecbb476

[nsan] Remove const qualifier from uint64_t args

d9fc0a8

[nsan] Simplify helper classes

ca874b7

Use one class instead of base and derived classes

[nsan] Pass number of fallback fn args as ctor param

15ad9e3

[nsan] Remove unneeded include

6eb74c6

[compiler-rt][nsan] Fix wrong formatting

501c004

clang-format cannot recognize that this is multiplication, not pointer declaration. See llvm#99271

[nsan] Calculate function type once

f17a0d5

With this approach the constructor of `NsanMemOpFn` can be simplified

[nsan] Update comment

82823ed

[nsan] Correct code style

6ee78ad

[nsan] Remove template parameter and refactor code

e66d162

[nsan] Remove wrong comments

666ec86

[nsan] Reorganize work with SmallVector

f3354b3

chestnykh force-pushed the nsan-optimized-instrumentation branch 2 times, most recently from 856adaf to d947559 Compare July 24, 2024 06:59

[nsan] Drop unneeded comment

d947559

chestnykh merged commit ddf5725 into llvm:main Jul 24, 2024
6 of 7 checks passed

hctim reviewed Jul 24, 2024

View reviewed changes

[nsan] Emit calls to optimized functions #98900

[nsan] Emit calls to optimized functions #98900

Uh oh!

Conversation

chestnykh commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chestnykh commented Jul 15, 2024

Uh oh!

chestnykh commented Jul 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chestnykh Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-shaposhnikov Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-shaposhnikov Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chestnykh Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chestnykh Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chestnykh commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chestnykh commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-shaposhnikov Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-shaposhnikov Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

chestnykh commented Jul 15, 2024 •

edited

Loading

llvmbot commented Jul 15, 2024 •

edited

Loading

chestnykh Jul 16, 2024 •

edited

Loading

alexander-shaposhnikov Jul 16, 2024 •

edited

Loading

alexander-shaposhnikov Jul 16, 2024 •

edited

Loading

chestnykh Jul 17, 2024 •

edited

Loading

chestnykh Jul 17, 2024 •

edited

Loading

chestnykh commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024 •

edited

Loading

chestnykh commented Jul 17, 2024 •

edited

Loading

alexander-shaposhnikov Jul 17, 2024 •

edited

Loading

alexander-shaposhnikov Jul 18, 2024 •

edited

Loading

alexander-shaposhnikov Jul 18, 2024 •

edited

Loading