[ASan] Add metadata to renamed instructions so ASan doesn't use the i… #119387

gbMattN · 2024-12-10T15:06:46Z

…ncorrect name

Clang needs variables to be represented with unique names. This means that if a variable shadows another, its given a different name internally to ensure it has a unique name. If ASan tries to use this name when printing an error, it will print the modified unique name, rather than the variable's source code name

Fixes #47326

github-actions · 2024-12-10T15:12:00Z

✅ With the latest revision this PR passed the C/C++ code formatter.

goussepi

Do you intend to add a test before posting review ? Otherwise LGTM!

llvm/lib/IR/ValueSymbolTable.cpp

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

gbMattN · 2024-12-10T15:24:36Z

Ah, yes! I'm also planning on getting some opinions on an RFC in case I have missed something
Edit: Here it is

gbMattN · 2024-12-11T17:16:01Z

The failing tests (that were not failing before my changes) are doing so because of additional metadata in the DWARF output. I'm waiting until after the exact metadata is finalized before fixing that. I don't want to add something to the tests that will need to be changed later if the metadata is renamed/altered.

llvmbot · 2024-12-18T11:24:33Z

@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-clang

Author: None (gbMattN)

Changes

…ncorrect name

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

4 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+8)
(added) compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp (+13)
(modified) llvm/include/llvm/IR/FixedMetadataKinds.def (+1)
(modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+9-7)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 5fccc9cbb37ec1..d8fdacf30e12e3 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -137,6 +137,14 @@ llvm::AllocaInst *CodeGenFunction::CreateTempAlloca(llvm::Type *Ty,
     Alloca =
         new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
                              ArraySize, Name, AllocaInsertPt->getIterator());
+  if (Alloca->getName() != Name.str() &&
+      SanOpts.Mask & SanitizerKind::Address) {
+
+    llvm::LLVMContext &ctx = Alloca->getContext();
+    llvm::MDString *trueNameMetadata = llvm::MDString::get(ctx, Name.str());
+    llvm::MDTuple *tuple = llvm::MDTuple::get(ctx, trueNameMetadata);
+    Alloca->setMetadata(llvm::LLVMContext::MD_unaltered_name, tuple);
+  }
   if (Allocas) {
     Allocas->Add(Alloca);
   }
diff --git a/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp b/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
new file mode 100644
index 00000000000000..f4d9ad5f6ea5f7
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
@@ -0,0 +1,13 @@
+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s
+
+int main()
+{
+	int x;
+	{
+		int x;
+		delete &x;
+	}
+}
+
+// CHECK: [32, 36) 'x'
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index df572e8791e13b..41fa34bf09ff65 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -53,3 +53,4 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38)
 LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39)
 LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40)
 LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41)
+LLVM_FIXED_MD_KIND(MD_unaltered_name, "unaltered.name", 42)
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index cb84588318496c..87f79bdaa16429 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -3430,13 +3430,15 @@ void FunctionStackPoisoner::processStaticAllocas() {
   SmallVector<ASanStackVariableDescription, 16> SVD;
   SVD.reserve(AllocaVec.size());
   for (AllocaInst *AI : AllocaVec) {
-    ASanStackVariableDescription D = {AI->getName().data(),
-                                      ASan.getAllocaSizeInBytes(*AI),
-                                      0,
-                                      AI->getAlign().value(),
-                                      AI,
-                                      0,
-                                      0};
+    const char *Name = AI->getName().data();
+    if (AI->hasMetadata(LLVMContext::MD_unaltered_name)) {
+      MDTuple *tuple =
+          dyn_cast<MDTuple>(AI->getMetadata(LLVMContext::MD_unaltered_name));
+      Name = dyn_cast<MDString>(tuple->getOperand(0))->getString().data();
+    }
+    ASanStackVariableDescription D = {
+        Name, ASan.getAllocaSizeInBytes(*AI), 0, AI->getAlign().value(), AI, 0,
+        0};
     SVD.push_back(D);
   }

llvmbot · 2024-12-18T11:24:33Z

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

Author: None (gbMattN)

Changes

…ncorrect name

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

4 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+8)
(added) compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp (+13)
(modified) llvm/include/llvm/IR/FixedMetadataKinds.def (+1)
(modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+9-7)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 5fccc9cbb37ec1..d8fdacf30e12e3 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -137,6 +137,14 @@ llvm::AllocaInst *CodeGenFunction::CreateTempAlloca(llvm::Type *Ty,
     Alloca =
         new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
                              ArraySize, Name, AllocaInsertPt->getIterator());
+  if (Alloca->getName() != Name.str() &&
+      SanOpts.Mask & SanitizerKind::Address) {
+
+    llvm::LLVMContext &ctx = Alloca->getContext();
+    llvm::MDString *trueNameMetadata = llvm::MDString::get(ctx, Name.str());
+    llvm::MDTuple *tuple = llvm::MDTuple::get(ctx, trueNameMetadata);
+    Alloca->setMetadata(llvm::LLVMContext::MD_unaltered_name, tuple);
+  }
   if (Allocas) {
     Allocas->Add(Alloca);
   }
diff --git a/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp b/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
new file mode 100644
index 00000000000000..f4d9ad5f6ea5f7
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
@@ -0,0 +1,13 @@
+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s
+
+int main()
+{
+	int x;
+	{
+		int x;
+		delete &x;
+	}
+}
+
+// CHECK: [32, 36) 'x'
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index df572e8791e13b..41fa34bf09ff65 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -53,3 +53,4 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38)
 LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39)
 LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40)
 LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41)
+LLVM_FIXED_MD_KIND(MD_unaltered_name, "unaltered.name", 42)
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index cb84588318496c..87f79bdaa16429 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -3430,13 +3430,15 @@ void FunctionStackPoisoner::processStaticAllocas() {
   SmallVector<ASanStackVariableDescription, 16> SVD;
   SVD.reserve(AllocaVec.size());
   for (AllocaInst *AI : AllocaVec) {
-    ASanStackVariableDescription D = {AI->getName().data(),
-                                      ASan.getAllocaSizeInBytes(*AI),
-                                      0,
-                                      AI->getAlign().value(),
-                                      AI,
-                                      0,
-                                      0};
+    const char *Name = AI->getName().data();
+    if (AI->hasMetadata(LLVMContext::MD_unaltered_name)) {
+      MDTuple *tuple =
+          dyn_cast<MDTuple>(AI->getMetadata(LLVMContext::MD_unaltered_name));
+      Name = dyn_cast<MDString>(tuple->getOperand(0))->getString().data();
+    }
+    ASanStackVariableDescription D = {
+        Name, ASan.getAllocaSizeInBytes(*AI), 0, AI->getAlign().value(), AI, 0,
+        0};
     SVD.push_back(D);
   }

goussepi · 2024-12-18T14:23:28Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

-                                      0};
+    const char *Name = AI->getName().data();
+    if (AI->hasMetadata(LLVMContext::MD_unaltered_name)) {
+      MDTuple *tuple =


Do we need to check the result of those dyn_cast are non null ? Otherwise LGTM!

We only add this metadata in one place, and expect it to have a MDString in it. If dyn_cast fails and we try doing this to a nullptr, LLVM will exit with a stack trace and request for a bug report, which I think is what we'd want to happen

gbMattN · 2025-01-07T13:33:16Z

@vitalybuka manually pinging you for a last review since you were active on the bugzilla ticket

fhahn · 2025-01-07T13:55:21Z

llvm/include/llvm/IR/FixedMetadataKinds.def

@@ -53,3 +53,4 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38)
 LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39)
 LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40)
 LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41)
+LLVM_FIXED_MD_KIND(MD_unaltered_name, "unaltered.name", 42)


Not sure if it is worth adding a new metadata kind to work around not using debug info?

If we need new metadata, it should be documented and also needs to be checked by the verifier, but perhaps it would be possible to use https://llvm.org/docs/LangRef.html#annotation-metadata instead?

Aha! I hadn't noticed that metadata type. I've switched to using it

fhahn · 2025-01-07T13:56:09Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

-                                      AI,
-                                      0,
-                                      0};
+    const char *Name = AI->getName().data();


Is is possible to use StringRef here?

fhahn · 2025-01-07T13:56:19Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

-                                      0};
+    const char *Name = AI->getName().data();
+    if (AI->hasMetadata(LLVMContext::MD_unaltered_name)) {
+      MDTuple *tuple =


Suggested change

MDTuple *tuple =

MDTuple *Tuple =

fhahn · 2025-01-07T13:57:35Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+          dyn_cast<MDTuple>(AI->getMetadata(LLVMContext::MD_unaltered_name));
+      Name = dyn_cast<MDString>(tuple->getOperand(0))->getString().data();


Should those be casts? If the dyn_cast can return nullptr, you need to check the return value

efriedma-quic · 2025-01-08T22:47:25Z

clang/lib/CodeGen/CGExpr.cpp

@@ -137,6 +137,10 @@ llvm::AllocaInst *CodeGenFunction::CreateTempAlloca(llvm::Type *Ty,
    Alloca =
        new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
                             ArraySize, Name, AllocaInsertPt->getIterator());
+  if (Alloca->getName() != Name.str() &&


I don't see the point of the Alloca->getName() != Name.str() check; it will almost always fail. (In release builds of clang, we suppress instruction names, so Alloca->getName() is just the empty string.)

It works with Release compiler.
https://godbolt.org/z/93ojPGM64

[32, 36) 'x' (line 7) <== Memory access at offset 32 is inside this variable

I don't see the point of the Alloca->getName() != Name.str() check; it will almost always fail. (In release builds of clang, we suppress instruction names, so Alloca->getName() is just the empty string.)

I'm still getting names from Allocas when I build in release mode, is there some other flag that does this?

Oh, we have some code that specifically messes with the default when asan is enabled.

llvm-project/clang/lib/Frontend/CompilerInvocation.cpp

Line 4973 in 3caa68a

Res.getCodeGenOpts().DiscardValueNames &=

If we have to support metadata anyway, I'd prefer to use the metadata unconditionally, instead of messing with LLVM IR instruction names.

@efriedma-quic To clarify we do:

set metadata always

remove DiscardValueNames hack (in a different future patch please, as this may cause regressions if we loose metdata more often than name)

LGTM if https://llvm-compile-time-tracker.com/ fail to notice a difference

I think they want the Alloca->getName() != Name.str() check removed, so that when running ASan, we emit this name metadata on every AllocaInst
I thought it would be nice to reduce the amount of additional metadata emitted, but I don't have particularly strong feelings on the matter

I don't think we need the metadata unless asan/msan is enabled. Debug info stores names in its own way, and I don't think we need the names for anything else... at least, I can't think of any other use.

The Alloca->getName() != Name.str() isn't going to work reliably: for example, inlining will rename values.

Thanks both for clarification, I misinterpreted the comment.

Suggested approach is definitely LGTM.

Alloca->getName() != Name.str() is still here?

Ah, I misunderstood. Check has been removed!

vitalybuka · 2025-01-09T01:22:57Z

I don't see (from description) why it's needed.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

gbMattN · 2025-01-09T14:16:56Z

I don't see (from description) why it's needed.

A reproducer of the issue can be seen failing on godbolt here
This is based off this bugzilla ticket

I forgot to put this at the top when I undrafted the pull request, apologies for the confusion.

vitalybuka · 2025-01-10T23:57:36Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+        for (int i = 0; i < Tuple->getNumOperands(); i++) {
+          if (auto stringMetadata = dyn_cast<MDString>(Tuple->getOperand(i))) {
+            if (stringMetadata->getString() == "alloca_name_altered") {
+              Name = ((MDString *)Tuple->getOperand(i + 1).get())->getString();


break or better return here

fhahn · 2025-01-20T13:34:51Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+    for (int i = 0; i < Annotation->getNumOperands(); i++) {
+      if (auto Tuple = dyn_cast<MDTuple>(Annotation->getOperand(i))) {
+        for (int i = 0; i < Tuple->getNumOperands(); i++) {
+          if (auto stringMetadata = dyn_cast<MDString>(Tuple->getOperand(i))) {
+            if (stringMetadata->getString() == "alloca_name_altered") {
+              return ((MDString *)Tuple->getOperand(i + 1).get())->getString();
+            }


There should be an easy way to do this :)

Might be good to use some early continues to reduce the nesting here.

fhahn · 2025-01-20T13:35:14Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+  if (AI->hasMetadata(LLVMContext::MD_annotation)) {
+    MDTuple *Annotation =
+        (MDTuple *)AI->getMetadata(LLVMContext::MD_annotation);
+    for (int i = 0; i < Annotation->getNumOperands(); i++) {


Can we directly iterate over the operands?

fhahn · 2025-01-20T13:35:35Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+  // recorded as an annotation.
+  if (AI->hasMetadata(LLVMContext::MD_annotation)) {
+    MDTuple *Annotation =
+        (MDTuple *)AI->getMetadata(LLVMContext::MD_annotation);


don't use old-style casts, this should probably be cast<>

fhahn · 2025-01-20T13:36:11Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+        (MDTuple *)AI->getMetadata(LLVMContext::MD_annotation);
+    for (int i = 0; i < Annotation->getNumOperands(); i++) {
+      if (auto Tuple = dyn_cast<MDTuple>(Annotation->getOperand(i))) {
+        for (int i = 0; i < Tuple->getNumOperands(); i++) {


Local variables usually start with upper case in LLVM (here and below)

fhahn · 2025-01-20T13:36:18Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

@@ -3391,6 +3391,27 @@ static void findStoresToUninstrumentedArgAllocas(
  }
 }

+StringRef getAllocaName(AllocaInst *AI) {


Suggested change

StringRef getAllocaName(AllocaInst *AI) {

static StringRef getAllocaName(AllocaInst *AI) {

fhahn · 2025-01-20T13:36:28Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+        for (int i = 0; i < Tuple->getNumOperands(); i++) {
+          if (auto stringMetadata = dyn_cast<MDString>(Tuple->getOperand(i))) {
+            if (stringMetadata->getString() == "alloca_name_altered") {
+              return ((MDString *)Tuple->getOperand(i + 1).get())->getString();


old style cast

fhahn · 2025-01-20T13:42:22Z

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

+  if (LocalEscapeCall)
+    LocalEscapeCall->moveBefore(InsBefore);


unrelated change?

Good catch, I think I was overzealous on the clang-format

fhahn · 2025-01-20T13:42:36Z

clang/lib/CodeGen/CGExpr.cpp

+  llvm::errs() << "Alloca name '" << Alloca->getName() << "'\n";
+  llvm::errs() << "NAME str '" << Name.str() << "'\n";


left over dbg ?

…ncorrect name

aaronpuchert · 2025-04-03T23:19:39Z

Maybe the test needs to be relaxed a bit because of stack layout differences in other OS targets? Although I'm not sure why they're different. See https://lab.llvm.org/buildbot/#/builders/186/builds/7896:

******************** TEST 'AddressSanitizer-arm-android :: TestCases/shadowed-stack-serialization.cpp' FAILED ********************
Exit Code: 1
Command Output (stderr):
--
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py  /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64  -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta  -fuse-ld=lld  -shared-libasan -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp # RUN: at line 1
+ /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -fuse-ld=lld -shared-libasan -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp
not  /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp 2>&1 | FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp # RUN: at line 2
+ not /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp
+ FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp:12:11: error: CHECK: expected string not found in input
// CHECK: [32, 36) 'x'
          ^
<stdin>:1:1: note: scanning from here
=================================================================
^
<stdin>:11:2: note: possible intended match here
 [16, 20) 'x' (line 7) <== Memory access at offset 16 is inside this variable
 ^
Input file: <stdin>
Check file: /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp

rorth · 2025-04-04T07:34:27Z

The patch also breaks both the Solaris/sparcv9 and Solaris/amd64 bots.

mstorsjo · 2025-04-04T08:52:28Z

Maybe the test needs to be relaxed a bit because of stack layout differences in other OS targets? Although I'm not sure why they're different. See https://lab.llvm.org/buildbot/#/builders/186/builds/7896:

******************** TEST 'AddressSanitizer-arm-android :: TestCases/shadowed-stack-serialization.cpp' FAILED ********************
Exit Code: 1
Command Output (stderr):
--
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py  /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang  --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only  --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64  -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta  -fuse-ld=lld  -shared-libasan -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp # RUN: at line 1
+ /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/sanitizer_common/android_commands/android_compile.py /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only --target=armv7-linux-androideabi24 --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64/sysroot --gcc-toolchain=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/toolchains/llvm/prebuilt/linux-x86_64 -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta -fuse-ld=lld -shared-libasan -O0 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp
not  /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp 2>&1 | FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp # RUN: at line 2
+ not /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_arm/test/asan/ARMAndroidConfig/TestCases/Output/shadowed-stack-serialization.cpp.tmp
+ FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp
/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp:12:11: error: CHECK: expected string not found in input
// CHECK: [32, 36) 'x'
          ^
<stdin>:1:1: note: scanning from here
=================================================================
^
<stdin>:11:2: note: possible intended match here
 [16, 20) 'x' (line 7) <== Memory access at offset 16 is inside this variable
 ^
Input file: <stdin>
Check file: /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm-project/compiler-rt/test/asan/TestCases/shadowed-stack-serialization.cpp

I'm also hitting this failure; in my case it's cropping up on i686 windows: https://github.com/mstorsjo/llvm-mingw/actions/runs/14255796193/job/39971583775

gbMattN · 2025-04-04T09:12:33Z

You're right, I don't think there is a reason to check the stack location in this test, just how it was serialized. I'll push a change and hopefully this'll be fixed. Thanks for the notes!

mstorsjo · 2025-04-04T10:19:47Z

You're right, I don't think there is a reason to check the stack location in this test, just how it was serialized. I'll push a change and hopefully this'll be fixed. Thanks for the notes!

Thanks for the fix in 4da5e9d, that does seem to have fixed the issue on i686 windows at least!

rorth · 2025-04-04T13:40:29Z

Both Solaris bots are back green again. Thanks.

goussepi approved these changes Dec 10, 2024

View reviewed changes

llvm/lib/IR/ValueSymbolTable.cpp Outdated Show resolved Hide resolved

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp Outdated Show resolved Hide resolved

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch from 01be0de to 8781ff2 Compare December 10, 2024 15:27

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch 2 times, most recently from 02ab9e5 to c15ec2f Compare December 17, 2024 17:03

gbMattN requested a review from goussepi December 18, 2024 11:23

gbMattN marked this pull request as ready for review December 18, 2024 11:23

llvmbot added clang Clang issues not falling into any other category compiler-rt clang:codegen IR generation bugs: mangling, exceptions, etc. compiler-rt:asan Address sanitizer compiler-rt:sanitizer llvm:ir llvm:transforms labels Dec 18, 2024

goussepi reviewed Dec 18, 2024

View reviewed changes

fhahn requested a review from vitalybuka January 7, 2025 13:37

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch from 6ca3a9e to 3a5c89a Compare January 7, 2025 13:41

fhahn reviewed Jan 7, 2025

View reviewed changes

gbMattN requested review from fhahn and goussepi January 7, 2025 17:13

efriedma-quic reviewed Jan 8, 2025

View reviewed changes

vitalybuka reviewed Jan 9, 2025

View reviewed changes

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp Outdated Show resolved Hide resolved

gbMattN requested a review from vitalybuka January 10, 2025 11:46

vitalybuka reviewed Jan 10, 2025

View reviewed changes

fhahn reviewed Jan 20, 2025

View reviewed changes

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch from 80a2b79 to 305390c Compare January 20, 2025 15:04

gbMattN requested a review from vitalybuka January 20, 2025 16:25

vitalybuka approved these changes Feb 20, 2025

View reviewed changes

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch from 8aa5a34 to fb5ad58 Compare April 3, 2025 10:51

[ASan] Add metadata to renamed instructions so ASan doesn't use the i…

b0845c9

…ncorrect name

gbMattN force-pushed the users/nagym/fix-asan-shadowed-variable-mis-serialization branch from fb5ad58 to b0845c9 Compare April 3, 2025 12:35

gbMattN merged commit 59074a3 into llvm:main Apr 3, 2025
11 checks passed

gbMattN deleted the users/nagym/fix-asan-shadowed-variable-mis-serialization branch April 3, 2025 14:27

		dyn_cast<MDTuple>(AI->getMetadata(LLVMContext::MD_unaltered_name));
		Name = dyn_cast<MDString>(tuple->getOperand(0))->getString().data();

	StringRef getAllocaName(AllocaInst *AI) {
	static StringRef getAllocaName(AllocaInst *AI) {

		llvm::errs() << "Alloca name '" << Alloca->getName() << "'\n";
		llvm::errs() << "NAME str '" << Name.str() << "'\n";

[ASan] Add metadata to renamed instructions so ASan doesn't use the i… #119387

[ASan] Add metadata to renamed instructions so ASan doesn't use the i… #119387

Uh oh!

Conversation

gbMattN commented Dec 10, 2024 • edited by efriedma-quic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

goussepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gbMattN commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gbMattN commented Dec 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

llvmbot commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbMattN commented Jan 7, 2025

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!

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!

vitalybuka Jan 14, 2025 • 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!

vitalybuka commented Jan 9, 2025

Uh oh!

Uh oh!

gbMattN commented Jan 9, 2025

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!

gbMattN commented Dec 10, 2024 •

edited by efriedma-quic

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

gbMattN commented Dec 10, 2024 •

edited

Loading

llvmbot commented Dec 18, 2024 •

edited

Loading

vitalybuka Jan 14, 2025 •

edited

Loading