Changes to support running tests for Windows arm64 asan #66973

farzonl · 2023-09-21T03:02:33Z

Differentiate SANITIZER_WINDOWS64 for x64 and arm64
turn off interception tests that expect x86 assembly

llvmbot · 2023-09-21T03:03:38Z

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

Changes

Differentiate SANITIZER_WINDOWS64 for x64 and arm64
fix A Warning where asserts needs messages
turn of interception tests that expect x86 assembly
After this change I can now build compiler-rt\lib\asan\tests\AARCH64WindowsConfig\Asan-aarch64-inline-Noinst-Test.exe

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

4 Files Affected:

(modified) compiler-rt/lib/interception/interception_win.cpp (+3-3)
(modified) compiler-rt/lib/interception/tests/interception_win_test.cpp (+2)
(modified) compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h (+1-1)
(modified) compiler-rt/lib/sanitizer_common/sanitizer_platform.h (+8)

diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index b2ba40902347f46..1e178f516c49d8b 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -1,4 +1,4 @@
-//===-- interception_linux.cpp ----------------------------------*- C++ -*-===//
+//===-- interception_win.cpp ----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -462,7 +462,7 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
   return 4;
 #endif
 
-#if SANITIZER_WINDOWS64
+#if SANITIZER_WINDOWSx64
   if (memcmp((u8*)address, kPrologueWithShortJump1,
              sizeof(kPrologueWithShortJump1)) == 0 ||
       memcmp((u8*)address, kPrologueWithShortJump2,
@@ -544,7 +544,7 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
       return 7;
   }
 
-#if SANITIZER_WINDOWS64
+#if SANITIZER_WINDOWSx64
   switch (*(u8*)address) {
     case 0xA1:  // A1 XX XX XX XX XX XX XX XX :
                 //   movabs eax, dword ptr ds:[XXXXXXXX]
diff --git a/compiler-rt/lib/interception/tests/interception_win_test.cpp b/compiler-rt/lib/interception/tests/interception_win_test.cpp
index 9159ce405f2dc49..629cdf437f342ec 100644
--- a/compiler-rt/lib/interception/tests/interception_win_test.cpp
+++ b/compiler-rt/lib/interception/tests/interception_win_test.cpp
@@ -17,6 +17,7 @@
 // Too slow for debug build
 #if !SANITIZER_DEBUG
 #if SANITIZER_WINDOWS
+#if !SANITIZER_WINDOWSARM64
 
 #include <stdarg.h>
 
@@ -793,5 +794,6 @@ TEST(Interception, EmptyExportTable) {
 
 }  // namespace __interception
 
+#endif   // !SANITIZER_WINDOWSARM64
 #endif  // SANITIZER_WINDOWS
 #endif  // #if !SANITIZER_DEBUG
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
index fa43ac50c61e4f4..c1c7e4c4cda2c80 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h
@@ -636,7 +636,7 @@ class SizeClassAllocator64 {
   }
   uptr SpaceEnd() const { return  SpaceBeg() + kSpaceSize; }
   // kRegionSize should be able to satisfy the largest size class.
-  static_assert(kRegionSize >= SizeClassMap::kMaxSize);
+  static_assert(kRegionSize >= SizeClassMap::kMaxSize, "kRegionSize must satisfy largest class size");
   // kRegionSize must be <= 2^36, see CompactPtrT.
   COMPILER_CHECK((kRegionSize) <= (1ULL << (SANITIZER_WORDSIZE / 2 + 4)));
   // Call mmap for user memory with at least this size.
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
index c1ca5c9ca44783b..55f3d0b846853e6 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -260,6 +260,14 @@
 #  define SANITIZER_ARM64 0
 #endif
 
+#if SANITIZER_WINDOWS64 && SANITIZER_ARM64
+#  define SANITIZER_WINDOWSARM64 1
+#  define SANITIZER_WINDOWSx64 0
+#else
+#  define SANITIZER_WINDOWSARM64 0
+#  define SANITIZER_WINDOWSx64 1
+#endif
+
 #if SANITIZER_SOLARIS && SANITIZER_WORDSIZE == 32
 #  define SANITIZER_SOLARIS32 1
 #else

farzonl · 2023-09-21T03:09:31Z

@omjavaid @vitalybuka

When you have time can you please review? Thanks in advance!

omjavaid · 2023-10-25T01:57:38Z

compiler-rt/lib/interception/interception_win.cpp

@@ -1,4 +1,4 @@
-//===-- interception_linux.cpp ----------------------------------*- C++ -*-===//
+//===-- interception_win.cpp ----------------------------------*- C++ -*-===//


This change seems to have been fixed on main. Your patch requires rebase.

omjavaid · 2023-10-25T01:58:09Z

compiler-rt/lib/sanitizer_common/sanitizer_allocator_primary64.h

@@ -636,7 +636,7 @@ class SizeClassAllocator64 {
  }
  uptr SpaceEnd() const { return  SpaceBeg() + kSpaceSize; }
  // kRegionSize should be able to satisfy the largest size class.
-  static_assert(kRegionSize >= SizeClassMap::kMaxSize);
+  static_assert(kRegionSize >= SizeClassMap::kMaxSize, "kRegionSize must satisfy largest class size");


Likewise. This has been fixed on main.

omjavaid · 2023-10-25T04:35:33Z

compiler-rt/lib/interception/interception_win.cpp

@@ -462,7 +462,7 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
  return 4;
 #endif

-#if SANITIZER_WINDOWS64
+#if SANITIZER_WINDOWSx64


Kindly add a comment on why this is x64 only.

Added a comment. SANITIZER_WINDOWS64 means SANITIZER_WINDOWS_x64 in this case and on line 552 because those are windows64 cases that parse x86_64 assembly.

omjavaid · 2023-10-25T04:41:00Z

compiler-rt/lib/interception/tests/interception_win_test.cpp

@@ -17,6 +17,7 @@
 // Too slow for debug build
 #if !SANITIZER_DEBUG
 #if SANITIZER_WINDOWS
+#if !SANITIZER_WINDOWSARM64


Similarly a comment for the future readers of this code. Also consider change SANITIZER_WINDOWSARM64 to SANITIZER_WINDOWS_ARM64 or something shorter and readable.

Added a comment. Changed the name to SANITIZER_WINDOWS_ARM64. The reason to disable is because all these tests assume x86/x86_64 assembly. Keeping these on is just going to make the llvm checks run on tests that will always fail.

github-actions · 2023-10-25T05:26:54Z

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

vitalybuka

LGTM, but please address @omjavaid comments.

1. Differentiate SANITIZER_WINDOWS64 for x64 and arm64. 2. turn off interception tests that expect x86 assembly.

omjavaid

LGTM

farzonl · 2023-11-27T08:14:16Z

@omjavaid I don't have write access. Can you merge this for me?

mstorsjo · 2023-11-28T12:59:55Z

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

+#  define SANITIZER_WINDOWS_x64 0
+#else
+#  define SANITIZER_WINDOWS_ARM64 0
+#  define SANITIZER_WINDOWS_x64 1


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.)

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.

This change makes x64 enablement case explicit and adds an else case for 32bit which fixes a regression on Windows i386 asan builds introduced by llvm#66973.

This change makes x64 enablement case explicit and adds an else case for 32bit which fixes a regression on Windows i386 asan builds introduced by #66973.

llvmbot added compiler-rt compiler-rt:sanitizer labels Sep 21, 2023

omjavaid self-requested a review October 25, 2023 01:17

omjavaid requested changes Oct 25, 2023

View reviewed changes

vitalybuka self-requested a review October 25, 2023 02:00

omjavaid added the platform:windows label Oct 25, 2023

omjavaid reviewed Oct 25, 2023

View reviewed changes

farzonl force-pushed the bugfix/disablebrokentests branch from 2667c12 to 141c9aa Compare October 25, 2023 05:13

vitalybuka approved these changes Nov 1, 2023

View reviewed changes

farzonl force-pushed the bugfix/disablebrokentests branch 4 times, most recently from 6892ff7 to 77450d8 Compare November 13, 2023 18:53

farzonl requested a review from JDevlieghere as a code owner November 13, 2023 19:03

farzonl force-pushed the bugfix/disablebrokentests branch 4 times, most recently from 0e1cc49 to 130a82c Compare November 13, 2023 19:47

farzonl and others added 2 commits November 13, 2023 15:01

Changes to support running tests for Windows arm64 asan

885226f

1. Differentiate SANITIZER_WINDOWS64 for x64 and arm64. 2. turn off interception tests that expect x86 assembly.

apply clang format

116bbf2

farzonl force-pushed the bugfix/disablebrokentests branch from 130a82c to 116bbf2 Compare November 13, 2023 20:04

farzonl requested a review from omjavaid November 13, 2023 20:10

omjavaid approved these changes Nov 27, 2023

View reviewed changes

plotfi merged commit d79aee9 into llvm:main Nov 27, 2023
3 checks passed

mstorsjo reviewed Nov 28, 2023

View reviewed changes

farzonl deleted the bugfix/disablebrokentests branch November 28, 2023 14:09

farzonl mentioned this pull request Nov 28, 2023

[asan] Fix Windows i386 regression #73650

Merged

mstorsjo pushed a commit that referenced this pull request Nov 28, 2023

[asan] Fix Windows i386 regression (#73650)

50b9930

This change makes x64 enablement case explicit and adds an else case for 32bit which fixes a regression on Windows i386 asan builds introduced by #66973.

farzonl self-assigned this Mar 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to support running tests for Windows arm64 asan #66973

Changes to support running tests for Windows arm64 asan #66973

farzonl commented Sep 21, 2023 •

edited

llvmbot commented Sep 21, 2023

farzonl commented Sep 21, 2023

omjavaid Oct 25, 2023

omjavaid Oct 25, 2023

omjavaid Oct 25, 2023

farzonl Nov 13, 2023 •

edited

omjavaid Oct 25, 2023

farzonl Nov 13, 2023

github-actions bot commented Oct 25, 2023 •

edited

vitalybuka left a comment

omjavaid left a comment

farzonl commented Nov 27, 2023

mstorsjo Nov 28, 2023

farzonl Nov 28, 2023

		@@ -1,4 +1,4 @@
		//===-- interception_linux.cpp ----------------------------------- C++ --===//
		//===-- interception_win.cpp ----------------------------------- C++ --===//

Changes to support running tests for Windows arm64 asan #66973

Changes to support running tests for Windows arm64 asan #66973

Conversation

farzonl commented Sep 21, 2023 • edited

llvmbot commented Sep 21, 2023

farzonl commented Sep 21, 2023

omjavaid Oct 25, 2023

Choose a reason for hiding this comment

omjavaid Oct 25, 2023

Choose a reason for hiding this comment

omjavaid Oct 25, 2023

Choose a reason for hiding this comment

farzonl Nov 13, 2023 • edited

Choose a reason for hiding this comment

omjavaid Oct 25, 2023

Choose a reason for hiding this comment

farzonl Nov 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 25, 2023 • edited

vitalybuka left a comment

Choose a reason for hiding this comment

omjavaid left a comment

Choose a reason for hiding this comment

farzonl commented Nov 27, 2023

mstorsjo Nov 28, 2023

Choose a reason for hiding this comment

farzonl Nov 28, 2023

Choose a reason for hiding this comment

farzonl commented Sep 21, 2023 •

edited

farzonl Nov 13, 2023 •

edited

github-actions bot commented Oct 25, 2023 •

edited