[libunwind] [libunwind] Fix running tests with MSan (PR #67860)

Alexander Richardson via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 18 07:45:52 PDT 2023


https://github.com/arichardson updated https://github.com/llvm/llvm-project/pull/67860

>From e39c568ba557fbca5d3ef17f3d4f4d74f15e8205 Mon Sep 17 00:00:00 2001
From: Alex Richardson <alexrichardson at google.com>
Date: Fri, 29 Sep 2023 13:25:39 -0700
Subject: [PATCH 1/2] [libunwind] Fix running tests with MSan

In order to run tests with MSan we have silence two false-positives:

First, we have to tell the runtime that __unw_getcontext actually populates
the buffer with initialized data.
Ideally we would call __msan_unpoison directly from __unw_getcontext, but
this would require adding assembly calls for all possible configurations
which is rather error prone. We also can't implement __unw_getcontext as
a C function that calls into the assembly code since that would result
in potentially incorrect register values being saved. Instead, use a
wrapper macro that calls __msan_unpoison directly after __unw_getcontext.

And second, MSan does not intercept _dl_find_object, so we initialize
the struct to zero before calling _dl_find_object.

These two changes are sufficient to make the whole test suite pass my
x86_64 Linux system.
---
 libunwind/include/libunwind.h                | 18 ++++++++++++++++++
 libunwind/src/AddressSpace.hpp               |  3 +++
 libunwind/src/libunwind_ext.h                | 10 ++++++++++
 libunwind/test/bad_unwind_info.pass.cpp      |  3 ---
 libunwind/test/forceunwind.pass.cpp          |  3 ---
 libunwind/test/libunwind_01.pass.cpp         |  3 ---
 libunwind/test/libunwind_02.pass.cpp         |  3 ---
 libunwind/test/remember_state_leak.pass.sh.s |  3 ---
 libunwind/test/signal_frame.pass.cpp         |  3 ---
 libunwind/test/signal_unwind.pass.cpp        |  3 ---
 libunwind/test/unw_resume.pass.cpp           |  3 ---
 libunwind/test/unwind_leaffunction.pass.cpp  |  3 ---
 12 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index b2dae8feed9a3b5..a1b02c07a2c79e3 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -43,6 +43,12 @@
   #define LIBUNWIND_AVAIL
 #endif
 
+#if defined(__SANITIZE_MEMORY__) ||                                            \
+    (defined(__has_feature) && __has_feature(memory_sanitizer))
+#define LIBUNWIND_HAVE_MSAN
+#include <sanitizer/msan_interface.h>
+#endif
+
 #if defined(_WIN32) && defined(__SEH__)
   #define LIBUNWIND_CURSOR_ALIGNMENT_ATTR __attribute__((__aligned__(16)))
 #else
@@ -115,6 +121,18 @@ extern int unw_set_reg(unw_cursor_t *, unw_regnum_t, unw_word_t) LIBUNWIND_AVAIL
 extern int unw_set_fpreg(unw_cursor_t *, unw_regnum_t, unw_fpreg_t)  LIBUNWIND_AVAIL;
 extern int unw_resume(unw_cursor_t *) LIBUNWIND_AVAIL;
 
+#ifdef LIBUNWIND_HAVE_MSAN
+// unw_getcontext is implemented in assembly so it is rather difficult to
+// mark the MSan shadow as initialized from within the function. Instead we
+// use a macro wrapper when compiling with MSan to avoid false-positives.
+#define unw_getcontext(context)                                                \
+  ({                                                                           \
+    int _result = (unw_getcontext)(context);                                   \
+    __msan_unpoison((context), sizeof(unw_context_t));                         \
+    _result;                                                                   \
+  })
+#endif
+
 #ifdef __arm__
 /* Save VFP registers in FSTMX format (instead of FSTMD). */
 extern void unw_save_vfp_as_X(unw_cursor_t *) LIBUNWIND_AVAIL;
diff --git a/libunwind/src/AddressSpace.hpp b/libunwind/src/AddressSpace.hpp
index 1abbc822546878d..f3e625037a64c57 100644
--- a/libunwind/src/AddressSpace.hpp
+++ b/libunwind/src/AddressSpace.hpp
@@ -621,6 +621,9 @@ inline bool LocalAddressSpace::findUnwindSections(pint_t targetAddr,
   }
   // Try to find the unwind info using `dl_find_object`
   dl_find_object findResult;
+  // _dl_find_object should fully initialize this struct, but we zero it to
+  // be safe in case this is not true (and to keep MSan quite).
+  memset(&findResult, 0, sizeof(findResult));
   if (dlFindObject && dlFindObject((void *)targetAddr, &findResult) == 0) {
     if (findResult.dlfo_eh_frame == nullptr) {
       // Found an entry for `targetAddr`, but there is no unwind info.
diff --git a/libunwind/src/libunwind_ext.h b/libunwind/src/libunwind_ext.h
index 28db43a4f6eef20..d55d906ea1a066c 100644
--- a/libunwind/src/libunwind_ext.h
+++ b/libunwind/src/libunwind_ext.h
@@ -32,6 +32,16 @@ extern int __unw_set_reg(unw_cursor_t *, unw_regnum_t, unw_word_t);
 extern int __unw_set_fpreg(unw_cursor_t *, unw_regnum_t, unw_fpreg_t);
 extern int __unw_resume(unw_cursor_t *);
 
+#ifdef LIBUNWIND_HAVE_MSAN
+// See comment above unw_getcontext (libunwind.h) for why this is needed.
+#define __unw_getcontext(context)                                              \
+  ({                                                                           \
+    int _result = (__unw_getcontext)(context);                                 \
+    __msan_unpoison((context), sizeof(unw_context_t));                         \
+    _result;                                                                   \
+  })
+#endif
+
 #ifdef __arm__
 /* Save VFP registers in FSTMX format (instead of FSTMD). */
 extern void __unw_save_vfp_as_X(unw_cursor_t *);
diff --git a/libunwind/test/bad_unwind_info.pass.cpp b/libunwind/test/bad_unwind_info.pass.cpp
index b3284e8daed71f3..b2c6b03aa25dd9b 100644
--- a/libunwind/test/bad_unwind_info.pass.cpp
+++ b/libunwind/test/bad_unwind_info.pass.cpp
@@ -15,9 +15,6 @@
 // GCC doesn't support __attribute__((naked)) on AArch64.
 // UNSUPPORTED: gcc
 
-// Inline assembly is incompatible with MSAN.
-// UNSUPPORTED: msan
-
 #undef NDEBUG
 #include <assert.h>
 #include <libunwind.h>
diff --git a/libunwind/test/forceunwind.pass.cpp b/libunwind/test/forceunwind.pass.cpp
index 8c26551b6d0b678..319d49011bfdb95 100644
--- a/libunwind/test/forceunwind.pass.cpp
+++ b/libunwind/test/forceunwind.pass.cpp
@@ -9,9 +9,6 @@
 
 // REQUIRES: linux
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 // Basic test for _Unwind_ForcedUnwind.
 // See libcxxabi/test/forced_unwind* tests too.
 
diff --git a/libunwind/test/libunwind_01.pass.cpp b/libunwind/test/libunwind_01.pass.cpp
index 96f12d1fc393743..f6617d0beb8bbb7 100644
--- a/libunwind/test/libunwind_01.pass.cpp
+++ b/libunwind/test/libunwind_01.pass.cpp
@@ -10,9 +10,6 @@
 // TODO: Investigate this failure on x86_64 macOS back deployment
 // XFAIL: stdlib=apple-libc++ && target=x86_64-apple-macosx{{10.9|10.10|10.11|10.12|10.13|10.14|10.15|11.0|12.0}}
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 #include <libunwind.h>
 #include <stdlib.h>
 #include <stdio.h>
diff --git a/libunwind/test/libunwind_02.pass.cpp b/libunwind/test/libunwind_02.pass.cpp
index fc034378781a2fa..f91432990c7b4b2 100644
--- a/libunwind/test/libunwind_02.pass.cpp
+++ b/libunwind/test/libunwind_02.pass.cpp
@@ -7,9 +7,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 #undef NDEBUG
 #include <assert.h>
 #include <stdlib.h>
diff --git a/libunwind/test/remember_state_leak.pass.sh.s b/libunwind/test/remember_state_leak.pass.sh.s
index 63beb7e4701ec0b..47566ba0d3ea906 100644
--- a/libunwind/test/remember_state_leak.pass.sh.s
+++ b/libunwind/test/remember_state_leak.pass.sh.s
@@ -8,9 +8,6 @@
 
 # REQUIRES: target={{x86_64-.+-linux-gnu}}
 
-# Inline assembly isn't supported by Memory Sanitizer
-# UNSUPPORTED: msan
-
 # RUN: %{build} -no-pie
 # RUN: %{run}
 
diff --git a/libunwind/test/signal_frame.pass.cpp b/libunwind/test/signal_frame.pass.cpp
index e5409f6ce3d9973..d60f932207482dd 100644
--- a/libunwind/test/signal_frame.pass.cpp
+++ b/libunwind/test/signal_frame.pass.cpp
@@ -12,9 +12,6 @@
 // TODO: Investigate this failure on Apple
 // XFAIL: target={{.+}}-apple-{{.+}}
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 // UNSUPPORTED: libunwind-arm-ehabi
 
 // The AIX assembler does not support CFI directives, which
diff --git a/libunwind/test/signal_unwind.pass.cpp b/libunwind/test/signal_unwind.pass.cpp
index 954a5d4ba3db10d..fd2180415c743f2 100644
--- a/libunwind/test/signal_unwind.pass.cpp
+++ b/libunwind/test/signal_unwind.pass.cpp
@@ -10,9 +10,6 @@
 // Ensure that the unwinder can cope with the signal handler.
 // REQUIRES: target={{(aarch64|riscv64|s390x|x86_64)-.+linux.*}}
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 #undef NDEBUG
 #include <assert.h>
 #include <dlfcn.h>
diff --git a/libunwind/test/unw_resume.pass.cpp b/libunwind/test/unw_resume.pass.cpp
index 08e8d4edeaf2927..0bf264baf6762e6 100644
--- a/libunwind/test/unw_resume.pass.cpp
+++ b/libunwind/test/unw_resume.pass.cpp
@@ -10,9 +10,6 @@
 // Ensure that unw_resume() resumes execution at the stack frame identified by
 // cursor.
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 #include <libunwind.h>
 
 void test_unw_resume() {
diff --git a/libunwind/test/unwind_leaffunction.pass.cpp b/libunwind/test/unwind_leaffunction.pass.cpp
index 8c9912e3c38680b..330dc2fd4e627b8 100644
--- a/libunwind/test/unwind_leaffunction.pass.cpp
+++ b/libunwind/test/unwind_leaffunction.pass.cpp
@@ -10,9 +10,6 @@
 // Ensure that leaf function can be unwund.
 // REQUIRES: target={{(aarch64|riscv64|s390x|x86_64)-.+linux.*}}
 
-// TODO: Figure out why this fails with Memory Sanitizer.
-// XFAIL: msan
-
 #undef NDEBUG
 #include <assert.h>
 #include <dlfcn.h>

>From 4a7e720a6377314e37a78bcef8ea015d63a64c9a Mon Sep 17 00:00:00 2001
From: Alexander Richardson <mail at alexrichardson.me>
Date: Wed, 18 Oct 2023 15:45:46 +0100
Subject: [PATCH 2/2] Fix typo

Co-authored-by: Louis Dionne <ldionne.2 at gmail.com>
---
 libunwind/src/AddressSpace.hpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libunwind/src/AddressSpace.hpp b/libunwind/src/AddressSpace.hpp
index f3e625037a64c57..827f7503fc49771 100644
--- a/libunwind/src/AddressSpace.hpp
+++ b/libunwind/src/AddressSpace.hpp
@@ -622,7 +622,7 @@ inline bool LocalAddressSpace::findUnwindSections(pint_t targetAddr,
   // Try to find the unwind info using `dl_find_object`
   dl_find_object findResult;
   // _dl_find_object should fully initialize this struct, but we zero it to
-  // be safe in case this is not true (and to keep MSan quite).
+  // be safe in case this is not true (and to keep MSan quiet).
   memset(&findResult, 0, sizeof(findResult));
   if (dlFindObject && dlFindObject((void *)targetAddr, &findResult) == 0) {
     if (findResult.dlfo_eh_frame == nullptr) {



More information about the cfe-commits mailing list