[AddressSanitizer] Remove memory effects from functions #130495

guy-david · 2025-03-09T16:30:50Z

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case:

  store i8 4, ptr %shadow
  call void @llvm.lifetime.start.p0(i64 4, ptr %local)
  %28 = call void @foo(ptr %local)
  store i8 -8, ptr %shadow
  call void @llvm.lifetime.end.p0(i64 4, ptr %local)

where foo is an external function with memory(argmem: write). A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within foo.

My first attempt was to add a memory(readwrite) at the call-site level, but unfortunately the current implementation of getMemoryEffects doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.

llvmbot · 2025-03-09T16:31:20Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-amdgpu

Author: Guy David (guy-david)

Changes

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case:

  store i8 4, ptr %shadow
  call void @<!-- -->llvm.lifetime.start.p0(i64 4, ptr %local)
  %28 = call void @<!-- -->foo(ptr %local)
  store i8 -8, ptr %shadow
  call void @<!-- -->llvm.lifetime.end.p0(i64 4, ptr %local)

where foo is an external function with memory(argmem: write). A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within foo.

My first attempt was to add a memory(readwrite) at the call-site level, but unfortunately the current implementation of getMemoryEffects doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.

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

5 Files Affected:

(modified) llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h (+4)
(modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+43)
(modified) llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp (+1-42)
(modified) llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll (+3-3)
(added) llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll (+23)

diff --git a/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h b/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
index 9fe2716220e83..154b37a535172 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
@@ -58,6 +58,10 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
                                bool IsKasan, uint64_t *ShadowBase,
                                int *MappingScale, bool *OrShadowOffset);
 
+/// Remove memory attributes that are incompatible with the instrumentation
+/// added by AddressSanitizer. See more details in the implementation.
+void removeFnAttributes(Function &F);
+
 } // namespace llvm
 
 #endif
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index b5e9cde2ba5f6..a2bc8b1de2fee 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -612,6 +612,46 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
   *OrShadowOffset = Mapping.OrShadowOffset;
 }
 
+void removeFnAttributes(Function &F) {
+  // Remove memory attributes that are invalid with ASan.
+  // ASan checks read from shadow, which invalidates memory(argmem: *)
+  // Short granule checks on function arguments read from the argument memory
+  // (last byte of the granule), which invalidates writeonly.
+  //
+  // This is not only true for sanitized functions, because AttrInfer can
+  // infer those attributes on libc functions, which is not true if those
+  // are instrumented (Android) or intercepted.
+  //
+  // We might want to model ASan shadow memory more opaquely to get rid of
+  // this problem altogether, by hiding the shadow memory write in an
+  // intrinsic, essentially like in the AArch64StackTagging pass. But that's
+  // for another day.
+
+  // The API is weird. `onlyReadsMemory` actually means "does not write", and
+  // `onlyWritesMemory` actually means "does not read". So we reconstruct
+  // "accesses memory" && "does not read" <=> "writes".
+  bool Changed = false;
+  if (!F.doesNotAccessMemory()) {
+    bool WritesMemory = !F.onlyReadsMemory();
+    bool ReadsMemory = !F.onlyWritesMemory();
+    if ((WritesMemory && !ReadsMemory) || F.onlyAccessesArgMemory()) {
+      F.removeFnAttr(Attribute::Memory);
+      Changed = true;
+    }
+  }
+  for (Argument &A : F.args()) {
+    if (A.hasAttribute(Attribute::WriteOnly)) {
+      A.removeAttr(Attribute::WriteOnly);
+      Changed = true;
+    }
+  }
+  if (Changed) {
+    // nobuiltin makes sure later passes don't restore assumptions about
+    // the function.
+    F.addFnAttr(Attribute::NoBuiltin);
+  }
+}
+
 ASanAccessInfo::ASanAccessInfo(int32_t Packed)
     : Packed(Packed),
       AccessSizeIndex((Packed >> kAccessSizeIndexShift) & kAccessSizeIndexMask),
@@ -2731,6 +2771,9 @@ GlobalVariable *ModuleAddressSanitizer::getOrCreateModuleName() {
 bool ModuleAddressSanitizer::instrumentModule() {
   initializeCallbacks();
 
+  for (Function &F : M)
+    removeFnAttributes(F);
+
   // Create a module constructor. A destructor is created lazily because not all
   // platforms, and not all modules need it.
   if (ConstructorKind == AsanCtorKind::Global) {
diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index e0e906d1eef7c..217f440a24a08 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -325,7 +325,6 @@ class HWAddressSanitizer {
                                           FunctionAnalysisManager &FAM) const;
   void initializeModule();
   void createHwasanCtorComdat();
-  void removeFnAttributes(Function *F);
 
   void initializeCallbacks(Module &M);
 
@@ -620,46 +619,6 @@ void HWAddressSanitizer::createHwasanCtorComdat() {
   appendToCompilerUsed(M, Dummy);
 }
 
-void HWAddressSanitizer::removeFnAttributes(Function *F) {
-  // Remove memory attributes that are invalid with HWASan.
-  // HWASan checks read from shadow, which invalidates memory(argmem: *)
-  // Short granule checks on function arguments read from the argument memory
-  // (last byte of the granule), which invalidates writeonly.
-  //
-  // This is not only true for sanitized functions, because AttrInfer can
-  // infer those attributes on libc functions, which is not true if those
-  // are instrumented (Android) or intercepted.
-  //
-  // We might want to model HWASan shadow memory more opaquely to get rid of
-  // this problem altogether, by hiding the shadow memory write in an
-  // intrinsic, essentially like in the AArch64StackTagging pass. But that's
-  // for another day.
-
-  // The API is weird. `onlyReadsMemory` actually means "does not write", and
-  // `onlyWritesMemory` actually means "does not read". So we reconstruct
-  // "accesses memory" && "does not read" <=> "writes".
-  bool Changed = false;
-  if (!F->doesNotAccessMemory()) {
-    bool WritesMemory = !F->onlyReadsMemory();
-    bool ReadsMemory = !F->onlyWritesMemory();
-    if ((WritesMemory && !ReadsMemory) || F->onlyAccessesArgMemory()) {
-      F->removeFnAttr(Attribute::Memory);
-      Changed = true;
-    }
-  }
-  for (Argument &A : F->args()) {
-    if (A.hasAttribute(Attribute::WriteOnly)) {
-      A.removeAttr(Attribute::WriteOnly);
-      Changed = true;
-    }
-  }
-  if (Changed) {
-    // nobuiltin makes sure later passes don't restore assumptions about
-    // the function.
-    F->addFnAttr(Attribute::NoBuiltin);
-  }
-}
-
 /// Module-level initialization.
 ///
 /// inserts a call to __hwasan_init to the module's constructor list.
@@ -668,7 +627,7 @@ void HWAddressSanitizer::initializeModule() {
   TargetTriple = M.getTargetTriple();
 
   for (Function &F : M.functions())
-    removeFnAttributes(&F);
+    removeFnAttributes(F);
 
   // x86_64 currently has two modes:
   // - Intel LAM (default)
diff --git a/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll b/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
index 3f62df76c0cf8..b46664fcaabbc 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
@@ -186,7 +186,7 @@ declare void @llvm.memset.p5.i32(ptr addrspace(5) nocapture writeonly, i8, i32,
 
 define void @test_mem_intrinsic_memcpy(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memcpy(ptr [[DEST0]], ptr [[SRC0]], i64 64)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -469,7 +469,7 @@ entry:
 
 define void @test_mem_intrinsic_memmove(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memmove(ptr [[DEST0]], ptr [[SRC0]], i64 64)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -753,7 +753,7 @@ entry:
 
 define void @test_mem_intrinsic_memset(ptr %ptr0,ptr addrspace(1) %ptr1,ptr addrspace(2) %ptr2,ptr addrspace(3) %ptr3,ptr addrspace(4) %ptr4,ptr addrspace(5) %ptr5) #0{
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memset(ptr [[PTR0]], i32 1, i64 128)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[PTR1]] to ptr
diff --git a/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll b/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll
new file mode 100644
index 0000000000000..ad91b88563ccb
--- /dev/null
+++ b/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll
@@ -0,0 +1,23 @@
+; Remove possible memory effects from functions that are invalidated by
+; AddressSanitizer instrumentation.
+
+; RUN: opt < %s -passes=asan -asan-use-after-scope -S | FileCheck %s
+
+target datalayout = "e-i64:64-f80:128-s:64-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: @foo(ptr) #[[ATTRS_FOO:[0-9]+]]
+declare void @foo(ptr) memory(argmem: write)
+
+; CHECK: @bar() #[[ATTRS_BAR:[0-9]+]]
+define void @bar() sanitize_address {
+entry:
+  %x = alloca i32, align 4
+  call void @llvm.lifetime.start.p0(i64 4, ptr %x)
+  call void @foo(ptr %x)
+  call void @llvm.lifetime.end.p0(i64 4, ptr %x)
+  ret void
+}
+
+; CHECK: attributes #[[ATTRS_FOO]] = { nobuiltin }
+; CHECK: attributes #[[ATTRS_BAR]] = { sanitize_address }

llvmbot · 2025-03-09T16:31:21Z

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

Author: Guy David (guy-david)

Changes

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case:

  store i8 4, ptr %shadow
  call void @<!-- -->llvm.lifetime.start.p0(i64 4, ptr %local)
  %28 = call void @<!-- -->foo(ptr %local)
  store i8 -8, ptr %shadow
  call void @<!-- -->llvm.lifetime.end.p0(i64 4, ptr %local)

where foo is an external function with memory(argmem: write). A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within foo.

My first attempt was to add a memory(readwrite) at the call-site level, but unfortunately the current implementation of getMemoryEffects doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.

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

5 Files Affected:

(modified) llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h (+4)
(modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+43)
(modified) llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp (+1-42)
(modified) llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll (+3-3)
(added) llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll (+23)

diff --git a/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h b/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
index 9fe2716220e83..154b37a535172 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
@@ -58,6 +58,10 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
                                bool IsKasan, uint64_t *ShadowBase,
                                int *MappingScale, bool *OrShadowOffset);
 
+/// Remove memory attributes that are incompatible with the instrumentation
+/// added by AddressSanitizer. See more details in the implementation.
+void removeFnAttributes(Function &F);
+
 } // namespace llvm
 
 #endif
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index b5e9cde2ba5f6..a2bc8b1de2fee 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -612,6 +612,46 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
   *OrShadowOffset = Mapping.OrShadowOffset;
 }
 
+void removeFnAttributes(Function &F) {
+  // Remove memory attributes that are invalid with ASan.
+  // ASan checks read from shadow, which invalidates memory(argmem: *)
+  // Short granule checks on function arguments read from the argument memory
+  // (last byte of the granule), which invalidates writeonly.
+  //
+  // This is not only true for sanitized functions, because AttrInfer can
+  // infer those attributes on libc functions, which is not true if those
+  // are instrumented (Android) or intercepted.
+  //
+  // We might want to model ASan shadow memory more opaquely to get rid of
+  // this problem altogether, by hiding the shadow memory write in an
+  // intrinsic, essentially like in the AArch64StackTagging pass. But that's
+  // for another day.
+
+  // The API is weird. `onlyReadsMemory` actually means "does not write", and
+  // `onlyWritesMemory` actually means "does not read". So we reconstruct
+  // "accesses memory" && "does not read" <=> "writes".
+  bool Changed = false;
+  if (!F.doesNotAccessMemory()) {
+    bool WritesMemory = !F.onlyReadsMemory();
+    bool ReadsMemory = !F.onlyWritesMemory();
+    if ((WritesMemory && !ReadsMemory) || F.onlyAccessesArgMemory()) {
+      F.removeFnAttr(Attribute::Memory);
+      Changed = true;
+    }
+  }
+  for (Argument &A : F.args()) {
+    if (A.hasAttribute(Attribute::WriteOnly)) {
+      A.removeAttr(Attribute::WriteOnly);
+      Changed = true;
+    }
+  }
+  if (Changed) {
+    // nobuiltin makes sure later passes don't restore assumptions about
+    // the function.
+    F.addFnAttr(Attribute::NoBuiltin);
+  }
+}
+
 ASanAccessInfo::ASanAccessInfo(int32_t Packed)
     : Packed(Packed),
       AccessSizeIndex((Packed >> kAccessSizeIndexShift) & kAccessSizeIndexMask),
@@ -2731,6 +2771,9 @@ GlobalVariable *ModuleAddressSanitizer::getOrCreateModuleName() {
 bool ModuleAddressSanitizer::instrumentModule() {
   initializeCallbacks();
 
+  for (Function &F : M)
+    removeFnAttributes(F);
+
   // Create a module constructor. A destructor is created lazily because not all
   // platforms, and not all modules need it.
   if (ConstructorKind == AsanCtorKind::Global) {
diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
index e0e906d1eef7c..217f440a24a08 100644
--- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -325,7 +325,6 @@ class HWAddressSanitizer {
                                           FunctionAnalysisManager &FAM) const;
   void initializeModule();
   void createHwasanCtorComdat();
-  void removeFnAttributes(Function *F);
 
   void initializeCallbacks(Module &M);
 
@@ -620,46 +619,6 @@ void HWAddressSanitizer::createHwasanCtorComdat() {
   appendToCompilerUsed(M, Dummy);
 }
 
-void HWAddressSanitizer::removeFnAttributes(Function *F) {
-  // Remove memory attributes that are invalid with HWASan.
-  // HWASan checks read from shadow, which invalidates memory(argmem: *)
-  // Short granule checks on function arguments read from the argument memory
-  // (last byte of the granule), which invalidates writeonly.
-  //
-  // This is not only true for sanitized functions, because AttrInfer can
-  // infer those attributes on libc functions, which is not true if those
-  // are instrumented (Android) or intercepted.
-  //
-  // We might want to model HWASan shadow memory more opaquely to get rid of
-  // this problem altogether, by hiding the shadow memory write in an
-  // intrinsic, essentially like in the AArch64StackTagging pass. But that's
-  // for another day.
-
-  // The API is weird. `onlyReadsMemory` actually means "does not write", and
-  // `onlyWritesMemory` actually means "does not read". So we reconstruct
-  // "accesses memory" && "does not read" <=> "writes".
-  bool Changed = false;
-  if (!F->doesNotAccessMemory()) {
-    bool WritesMemory = !F->onlyReadsMemory();
-    bool ReadsMemory = !F->onlyWritesMemory();
-    if ((WritesMemory && !ReadsMemory) || F->onlyAccessesArgMemory()) {
-      F->removeFnAttr(Attribute::Memory);
-      Changed = true;
-    }
-  }
-  for (Argument &A : F->args()) {
-    if (A.hasAttribute(Attribute::WriteOnly)) {
-      A.removeAttr(Attribute::WriteOnly);
-      Changed = true;
-    }
-  }
-  if (Changed) {
-    // nobuiltin makes sure later passes don't restore assumptions about
-    // the function.
-    F->addFnAttr(Attribute::NoBuiltin);
-  }
-}
-
 /// Module-level initialization.
 ///
 /// inserts a call to __hwasan_init to the module's constructor list.
@@ -668,7 +627,7 @@ void HWAddressSanitizer::initializeModule() {
   TargetTriple = M.getTargetTriple();
 
   for (Function &F : M.functions())
-    removeFnAttributes(&F);
+    removeFnAttributes(F);
 
   // x86_64 currently has two modes:
   // - Intel LAM (default)
diff --git a/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll b/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
index 3f62df76c0cf8..b46664fcaabbc 100644
--- a/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
+++ b/llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll
@@ -186,7 +186,7 @@ declare void @llvm.memset.p5.i32(ptr addrspace(5) nocapture writeonly, i8, i32,
 
 define void @test_mem_intrinsic_memcpy(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memcpy(ptr [[DEST0]], ptr [[SRC0]], i64 64)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -469,7 +469,7 @@ entry:
 
 define void @test_mem_intrinsic_memmove(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memmove(ptr [[DEST0]], ptr [[SRC0]], i64 64)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -753,7 +753,7 @@ entry:
 
 define void @test_mem_intrinsic_memset(ptr %ptr0,ptr addrspace(1) %ptr1,ptr addrspace(2) %ptr2,ptr addrspace(3) %ptr3,ptr addrspace(4) %ptr4,ptr addrspace(5) %ptr5) #0{
 entry:
-  ;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]) #2 {
+  ;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]){{.*}} {
   ;CHECK-NEXT: entry:
   ;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memset(ptr [[PTR0]], i32 1, i64 128)
   ;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[PTR1]] to ptr
diff --git a/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll b/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll
new file mode 100644
index 0000000000000..ad91b88563ccb
--- /dev/null
+++ b/llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll
@@ -0,0 +1,23 @@
+; Remove possible memory effects from functions that are invalidated by
+; AddressSanitizer instrumentation.
+
+; RUN: opt < %s -passes=asan -asan-use-after-scope -S | FileCheck %s
+
+target datalayout = "e-i64:64-f80:128-s:64-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: @foo(ptr) #[[ATTRS_FOO:[0-9]+]]
+declare void @foo(ptr) memory(argmem: write)
+
+; CHECK: @bar() #[[ATTRS_BAR:[0-9]+]]
+define void @bar() sanitize_address {
+entry:
+  %x = alloca i32, align 4
+  call void @llvm.lifetime.start.p0(i64 4, ptr %x)
+  call void @foo(ptr %x)
+  call void @llvm.lifetime.end.p0(i64 4, ptr %x)
+  ret void
+}
+
+; CHECK: attributes #[[ATTRS_FOO]] = { nobuiltin }
+; CHECK: attributes #[[ATTRS_BAR]] = { sanitize_address }

llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

arsenm · 2025-03-12T02:52:55Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+  bool Changed = false;
+  if (!F.doesNotAccessMemory()) {
+    bool WritesMemory = !F.onlyReadsMemory();
+    bool ReadsMemory = !F.onlyWritesMemory();
+    if ((WritesMemory && !ReadsMemory) || F.onlyAccessesArgMemory()) {
+      F.removeFnAttr(Attribute::Memory);
+      Changed = true;
+    }
+  }
+  if (RemoveWriteOnly) {
+    for (Argument &A : F.args()) {
+      if (A.hasAttribute(Attribute::WriteOnly)) {
+        A.removeAttr(Attribute::WriteOnly);
+        Changed = true;
+      }
+    }
+  }
+  if (Changed) {
+    // nobuiltin makes sure later passes don't restore assumptions about
+    // the function.
+    F.addFnAttr(Attribute::NoBuiltin);
+  }
+}


I see you're only moving this function, but this Attribute API usage could be cleaned up a lot. All of the pre-queries seem unnecessary, and are wrappers around checking the same attribute which you are already modifying. Should just naturally operate in terms of Memory rather than the old-style wrappers.

I'm not completely following, can you explain what it means to "operate in terms of Memory"?

F.removeFnAttr(Attribute::Memory); All of these other onlyXMemory checks are convenience wrappers. You could rewrite this to get the raw value for Attribute::Memory and mask that instead

I'll do so in a follow up PR.

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h

llvm/lib/Passes/PassBuilder.cpp

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h

llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll

llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

@foo

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case: ``` store i8 4, ptr %shadow call void @llvm.lifetime.start.p0(i64 4, ptr %local) %28 = call void @foo(ptr %local) store i8 -8, ptr %shadow call void @llvm.lifetime.end.p0(i64 4, ptr %local) ``` where `foo` is an external function with `memory(argmem: write)`. A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within `foo`.

fhahn

LGTM, thanks!

@foo

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case: ``` store i8 4, ptr %shadow call void @llvm.lifetime.start.p0(i64 4, ptr %local) %28 = call void @foo(ptr %local) store i8 -8, ptr %shadow call void @llvm.lifetime.end.p0(i64 4, ptr %local) ``` where `foo` is an external function with `memory(argmem: write)`. A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within `foo`. My first attempt was to add a `memory(readwrite)` at the call-site level, but unfortunately the current implementation of `getMemoryEffects` doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.

@foo

If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case: ``` store i8 4, ptr %shadow call void @llvm.lifetime.start.p0(i64 4, ptr %local) %28 = call void @foo(ptr %local) store i8 -8, ptr %shadow call void @llvm.lifetime.end.p0(i64 4, ptr %local) ``` where `foo` is an external function with `memory(argmem: write)`. A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within `foo`. My first attempt was to add a `memory(readwrite)` at the call-site level, but unfortunately the current implementation of `getMemoryEffects` doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.

guy-david requested a review from fmayer March 9, 2025 16:30

llvmbot added backend:AMDGPU compiler-rt:sanitizer llvm:transforms labels Mar 9, 2025

guy-david force-pushed the users/guy-david/asan-memory-effects branch from f97c90e to 5d2d839 Compare March 10, 2025 08:36

arsenm reviewed Mar 10, 2025

View reviewed changes

llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll Outdated Show resolved Hide resolved

llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll Show resolved Hide resolved

fhahn requested a review from vitalybuka March 10, 2025 10:13

fhahn reviewed Mar 10, 2025

View reviewed changes

guy-david force-pushed the users/guy-david/asan-memory-effects branch 2 times, most recently from 4f838ef to 7d2da91 Compare March 10, 2025 15:39

guy-david requested review from fhahn and arsenm March 10, 2025 15:47

thetruestblue requested review from thetruestblue and wrotki March 10, 2025 17:17

arsenm reviewed Mar 12, 2025

View reviewed changes

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h Outdated Show resolved Hide resolved

vitalybuka reviewed Mar 12, 2025

View reviewed changes

llvm/lib/Passes/PassBuilder.cpp Outdated Show resolved Hide resolved

guy-david force-pushed the users/guy-david/asan-memory-effects branch from 7d2da91 to 18531c2 Compare March 12, 2025 09:10

guy-david changed the base branch from main to users/guy-david/asan-pipeline-use-after-scope March 12, 2025 09:11

guy-david requested a review from arsenm March 12, 2025 09:21

Base automatically changed from users/guy-david/asan-pipeline-use-after-scope to main March 12, 2025 15:17

guy-david force-pushed the users/guy-david/asan-memory-effects branch from 18531c2 to 8c0b3c9 Compare March 13, 2025 09:40

fhahn reviewed Mar 13, 2025

View reviewed changes

guy-david force-pushed the users/guy-david/asan-memory-effects branch from 8c0b3c9 to c9e5fa8 Compare March 14, 2025 11:08

guy-david requested a review from fhahn March 14, 2025 11:08

fhahn approved these changes Mar 15, 2025

View reviewed changes

guy-david merged commit 3168110 into main Mar 15, 2025
11 checks passed

guy-david deleted the users/guy-david/asan-memory-effects branch March 15, 2025 18:55

tmiasko mentioned this pull request Mar 15, 2025

AddressSanitizer false positive due to incorrectly preserved writesonly attribute #56205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AddressSanitizer] Remove memory effects from functions #130495

[AddressSanitizer] Remove memory effects from functions #130495

Uh oh!

guy-david commented Mar 9, 2025 •

edited

Loading

Uh oh!

llvmbot commented Mar 9, 2025 •

edited

Loading

Uh oh!

llvmbot commented Mar 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Mar 12, 2025

Uh oh!

guy-david Mar 12, 2025

Uh oh!

arsenm Mar 13, 2025

Uh oh!

guy-david Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhahn left a comment

Uh oh!

Uh oh!

Uh oh!

[AddressSanitizer] Remove memory effects from functions #130495

[AddressSanitizer] Remove memory effects from functions #130495

Uh oh!

Conversation

guy-david commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

guy-david Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

arsenm Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

guy-david Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

guy-david commented Mar 9, 2025 •

edited

Loading

llvmbot commented Mar 9, 2025 •

edited

Loading