Skip to content

Changes to support running tests for Windows arm64 asan #66973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Changes to support running tests for Windows arm64 asan #66973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
885226f
Changes to support running tests for Windows arm64 asan
farzonl Sep 21, 2023
116bbf2
apply clang format
Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions compiler-rt/lib/interception/interception_win.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,8 @@ static uptr AllocateMemoryForTrampoline(uptr image_address, size_t size) {
// The following prologues cannot be patched because of the short jump
// jumping to the patching region.

#if SANITIZER_WINDOWS64
// Short jump patterns below are only for x86_64.
# if SANITIZER_WINDOWS_x64
// ntdll!wcslen in Win11
// 488bc1 mov rax,rcx
// 0fb710 movzx edx,word ptr [rax]
Expand Down Expand Up @@ -462,7 +463,7 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
return 4;
#endif

#if SANITIZER_WINDOWS64
# if SANITIZER_WINDOWS_x64
if (memcmp((u8*)address, kPrologueWithShortJump1,
sizeof(kPrologueWithShortJump1)) == 0 ||
memcmp((u8*)address, kPrologueWithShortJump2,
Expand Down Expand Up @@ -544,7 +545,7 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
return 7;
}

#if SANITIZER_WINDOWS64
# if SANITIZER_WINDOWS_x64
switch (*(u8*)address) {
case 0xA1: // A1 XX XX XX XX XX XX XX XX :
// movabs eax, dword ptr ds:[XXXXXXXX]
Expand Down
9 changes: 6 additions & 3 deletions compiler-rt/lib/interception/tests/interception_win_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
#include "gtest/gtest.h"

// Too slow for debug build
// Disabling for ARM64 since testcases are x86/x64 assembly.
#if !SANITIZER_DEBUG
#if SANITIZER_WINDOWS
# if !SANITIZER_WINDOWS_ARM64

#include <stdarg.h>
# include <stdarg.h>

#define WIN32_LEAN_AND_MEAN
#include <windows.h>
# define WIN32_LEAN_AND_MEAN
# include <windows.h>

namespace __interception {
namespace {
Expand Down Expand Up @@ -793,5 +795,6 @@ TEST(Interception, EmptyExportTable) {

} // namespace __interception

# endif // !SANITIZER_WINDOWS_ARM64
#endif // SANITIZER_WINDOWS
#endif // #if !SANITIZER_DEBUG
8 changes: 8 additions & 0 deletions compiler-rt/lib/sanitizer_common/sanitizer_platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,14 @@
# define SANITIZER_ARM64 0
#endif

#if SANITIZER_WINDOWS64 && SANITIZER_ARM64
# define SANITIZER_WINDOWS_ARM64 1
# define SANITIZER_WINDOWS_x64 0
#else
# define SANITIZER_WINDOWS_ARM64 0
# define SANITIZER_WINDOWS_x64 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This broke ASAN on Windows/i386. This is within a generic context, and doing #define SANITIZER_WINDOWS_x64 1 if #if SANITIZER_WINDOWS64 && SANITIZER_ARM64 doesn't hold, means that SANITIZER_WINDOWS_x64 gets enabled everywhere, on every platform, except for windows/arm64. This probably has little effect outside of interception_win.cpp though, which means that it breaks things for all windows architectures other than x86_64 and aarch64.

It's simple to fix though, e.g. like this:

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
index 49d8a67cc12d..34baf1bea58b 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -260,13 +260,18 @@
 #  define SANITIZER_ARM64 0
 #endif

-#if SANITIZER_WINDOWS64 && SANITIZER_ARM64
+#if SANITIZER_WINDOWS64
+#if SANITIZER_ARM64
 #  define SANITIZER_WINDOWS_ARM64 1
 #  define SANITIZER_WINDOWS_x64 0
 #else
 #  define SANITIZER_WINDOWS_ARM64 0
 #  define SANITIZER_WINDOWS_x64 1
 #endif
+#else
+#  define SANITIZER_WINDOWS_ARM64 0
+#  define SANITIZER_WINDOWS_x64 0
+#endif

 #if SANITIZER_SOLARIS && SANITIZER_WORDSIZE == 32
 #  define SANITIZER_SOLARIS32 1

Do people prefer me to revert this commit and you can redo it, or for me to just push this fix? (I can push either a revert or a fix later today.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a fix. Also could we use this patch instead:
https://patch-diff.githubusercontent.com/raw/llvm/llvm-project/pull/73650.patch
#73650

clang-format pr validation wants to indent the #if SANITIZER_ARM64 block didn't want to do that. my patch is functionally the same.

#endif

#if SANITIZER_SOLARIS && SANITIZER_WORDSIZE == 32
# define SANITIZER_SOLARIS32 1
#else
Expand Down