[libunwind] Fix running tests with MSan #67860
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libunwind ChangesIn 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. 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. Full diff: https://github.com/llvm/llvm-project/pull/67860.diff 12 Files Affected:
diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index b2dae8feed9a3b5..4b059ddd5e6fc79 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -43,6 +43,10 @@
#define LIBUNWIND_AVAIL
#endif
+#if __has_feature(memory_sanitizer) || defined(__SANITIZE_MEMORY__)
+#include <sanitizer/msan_interface.h>
+#endif
+
#if defined(_WIN32) && defined(__SEH__)
#define LIBUNWIND_CURSOR_ALIGNMENT_ATTR __attribute__((__aligned__(16)))
#else
@@ -115,6 +119,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;
+#if __has_feature(memory_sanitizer) || defined(__SANITIZE_MEMORY__)
+// 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..df43d5837cf1e39 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 *);
+#if __has_feature(memory_sanitizer) || defined(__SANITIZE_MEMORY__)
+// 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>
|
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.
arichardson
force-pushed
the
libunwind-msan-fixes
branch
from
74aad57
to
e39c568
Compare
ldionne
requested changes
Oct 10, 2023
| // 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) \ |
There was a problem hiding this comment.
Could we perhaps use an inline function instead of a macro?
There was a problem hiding this comment.
I don't think we can since I believe that having an exported symbol called unw_getcontext is required. But I'm not familiar with what level of compatibility with GNU libunwind is required.
There was a problem hiding this comment.
I think what I had in mind was something like:
extern "C" int __unw_getcontext(unw_context_t *);
inline int unw_getcontext(unw_context_t *ctx) {
// do stuff
return __unw_getcontext(ctx);
}
I am having a bit of trouble tracking down how unw_getcontext is implemented right now, but IIUC it's only an alias to __unw_getcontext, is that right? If so, I think the inline approach here would work, I think?
There was a problem hiding this comment.
We have to be careful to call unw_getcontext first to avoid registers being clobbered, but yes that would work if we don't need unw_getcontext to be an exported function. Maybe extern inline __attribute__((always_inline)) could work.
There was a problem hiding this comment.
I think I'd defer to the libunwind regulars. @MaskRay perhaps?
MaskRay
reviewed
Oct 18, 2023
| @@ -43,6 +43,12 @@ | |||
| #define LIBUNWIND_AVAIL | |||
| #endif | |||
|
|
|||
| #if defined(__SANITIZE_MEMORY__) || \ | |||
There was a problem hiding this comment.
__SANITIZE_MEMORY__ is a macro defined by the Linux kernel and copied to other projects.
We can drop it.
There was a problem hiding this comment.
Ah I assumed it was needed for GCC since it doesn't have __has_feature().
There was a problem hiding this comment.
GCC just doesn't support msan :)
There was a problem hiding this comment.
Sounds good. In that case I'll update the PR when I'm back in the office next Thursday.
|
Gentle ping! |
Sorry for the delay - I was busy with the RISC-V summit last week, hoping to get to this PR later this week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.