Fix registers for Windows on ARM64

[DoDoENT]

Added formatting changes requested by the build-bot.

[DoDoENT]

Another attempt in satisfying clang-format build-bot...

[DoDoENT]

Yet another attempt to fix formatting errors.

[DoDoENT]

Note that this change only makes the code compile. Unfortunately, sanitizers don't work on WOA64 even with this change 😢

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

61

There is also _M_ARM64

SANITIZER_WINDOWS64 and SANITIZER_ARM64 ?

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

966

same?

[DoDoENT]

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

61

What is the difference between _M_ARM64 and __arch64__? Isn't __aarch64__ supposed to be the standardized way of checking for ARM64 target? (It's my first contribution and I'm not so familiar with the codebase).

I see that some other source code checks for both __aarch64__ and _M_ARM64 - why are both checks required? Is there any documentation about that?

Regarding the SANITIZER_WINDOWS64 and SANITIZER_ARM64 - I haven't seen any mention of SANITIZER_ARM64 in the LLVM codebase and SANITIZER_WINDOWS64 should have already been used here even for x86_64, shouldn't it? Isn't that macro supposed to guard sanitizer-enabled builds vs non-sanitizer-enabled builds? Why is it then needed here in the implementation of the sanitizer runtime?

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

966

Same question/comment as above.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

61

What is the difference between _M_ARM64 and __arch64__? Isn't __aarch64__ supposed to be the standardized way of checking for ARM64 target? (It's my first contribution and I'm not so familiar with the codebase).

_M_ARM64 is MSVC thing https://godbolt.org/z/P16fYfM7q

I see that some other source code checks for both __aarch64__ and _M_ARM64 - why are both checks required? Is there any documentation about that?

compiler-rt and llvm code base are quite isolated
so please use compuler-rt/ for examples

Regarding the SANITIZER_WINDOWS64 and SANITIZER_ARM64 - I haven't seen any mention of SANITIZER_ARM64 in the LLVM codebase and SANITIZER_WINDOWS64 should have already been used here even for x86_64, shouldn't it? Isn't that macro supposed to guard sanitizer-enabled builds vs non-sanitizer-enabled builds? Why is it then needed here in the implementation of the sanitizer runtime?

SANITIZER_WINDOWS64 is defined in llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_platform.h which is sanitizer internals (implementation)
I would prefer SANITIZER_WINDOWS64 for consistency, but feel free to keep _WIN64 as-is, as you didn't introduce it int this patch.

[DoDoENT]

Update per review.

Also run clang-format utility on functions containing the changed lines.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

61

#    if SANITIZER_WINDOWS64
#      if SANITIZER_ARM64

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

965

same

[vitalybuka]

I assume you can't commit patches. I'll fix and commit for you.

[vitalybuka]

Lp -> Fp

[vitalybuka]

Can you please confirm that it compiles this way?

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

64

It should be Lr -> Sp ?

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

967

same?

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

64

It should be Lr -> Sp ?

Lr -> Fp

[DoDoENT]

In D116376#3228141, @vitalybuka wrote:

Can you please confirm that it compiles this way?

Yes! I've managed to compile the compile-rt module from the pre-merge commit resembling your changes. However, I cannot compile the clang anymore (unlike my commit based on llvm-13.0.0) as it fails with the following error:

[1/3559 4.0/sec] Building RC object utils\TableGen\CMakeFi...tblgen.dir\__\__\resources\windows_version_resource.rc.res
FAILED: utils/TableGen/CMakeFiles/llvm-tblgen.dir/__/__/resources/windows_version_resource.rc.res
RC Z:\Work\Microblink\llvm-project\llvm\resources\windows_version_resource.rc utils\TableGen\CMakeFiles\llvm-tblgen.dir\__\__\resources\windows_version_resource.rc.res.d utils\TableGen\CMakeFiles\llvm-tblgen.dir\__\__\resources\windows_version_resource.rc.res "Note: including file: " "Z:/.conan/data/llvm/13.0.0/microblink/stable/package/27d267decbd899b8f15d3767c082b6f76419549b/bin/clang-cl.exe" C:\PROGRA~2\WI3CF2~1\10\bin\100190~1.0\x64\rc.exe -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -DRC_FILE_VERSION=\"14.0.0git\" -DRC_INTERNAL_NAME=\"llvm-tblgen\" -DRC_PRODUCT_NAME=\"LLVM\" -DRC_PRODUCT_VERSION=\"14.0.0git\" -DRC_VERSION_FIELD_1=14 -DRC_VERSION_FIELD_2=0 -DRC_VERSION_FIELD_3=0 -DRC_VERSION_FIELD_4=0 -I C:\Users\dodo\Desktop\builds\llvm-build\utils\TableGen -I Z:\Work\Microblink\llvm-project\llvm\utils\TableGen -I C:\Users\dodo\Desktop\builds\llvm-build\include -I Z:\Work\Microblink\llvm-project\llvm\include -DWIN32 /nologo /fo utils\TableGen\CMakeFiles\llvm-tblgen.dir\__\__\resources\windows_version_resource.rc.res Z:\Work\Microblink\llvm-project\llvm\resources\windows_version_resource.rc
Microsoft (R) Windows (R) Resource Compiler Version 10.0.10011.16384
Copyright (C) Microsoft Corporation.  All rights reserved.

fatal error RC1107: invalid usage; use RC /? for Help

I guess this is unrelated to this diff and is something upstream (I'm using clang-cl to compile LLVM, not msvc, as I'm using Windows 11 on ARM within Parallels Desktop on M1 Pro, and there is no native MSVC for ARM64, and emulation is too slow, especially for compiling large codebases like LLVM).

However, I'll try to transplant built clang_rt libraries into my current LLVM 13.0.0 WOA64 distribution to test if it works... I'll let you know if this works (my first attempt didn't, but could be due to a bug that you fixed (Lr -> Fp).

[DoDoENT]

However, I'll try to transplant built clang_rt libraries into my current LLVM 13.0.0 WOA64 distribution to test if it works... I'll let you know if this works (my first attempt didn't, but could be due to a bug that you fixed (Lr -> Fp).

Unfortunately, this doesn't work - my binary that should trigger ASAN simply prints nothing out (neither normal output nor anything else). This is the same behavior I had prior to switching Lr and Fp... It's possible that this is due to using custom clang_rt_asan.dll in combination with pre-build LLVM 13.0.0 clang.

I've tried running under VS debugger, and it traps with the following stack trace:

[Inline Frame] clang_rt.asan_dynamic-aarch64.dll!__interception::InterceptionFailed() Line 146
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\interception\interception_win.cpp(146)
clang_rt.asan_dynamic-aarch64.dll!__interception::GetInstructionSize(unsigned __int64 address, unsigned __int64 * rel_offset) Line 600
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\interception\interception_win.cpp(600)
clang_rt.asan_dynamic-aarch64.dll!__interception::OverrideFunctionWithHotPatch(unsigned __int64 old_func, unsigned __int64 new_func, unsigned __int64 * orig_old_func) Line 776
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\interception\interception_win.cpp(776)
[Inline Frame] clang_rt.asan_dynamic-aarch64.dll!__interception::OverrideFunction(unsigned __int64 old_func, unsigned __int64 new_func, unsigned __int64 * orig_old_func) Line 881
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\interception\interception_win.cpp(881)
clang_rt.asan_dynamic-aarch64.dll!__interception::OverrideFunction(const char * func_name, unsigned __int64 new_func, unsigned __int64 * orig_old_func) Line 994
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\interception\interception_win.cpp(994)
[Inline Frame] clang_rt.asan_dynamic-aarch64.dll!InitializeCommonInterceptors() Line 10225
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\sanitizer_common\sanitizer_common_interceptors.inc(10225)
clang_rt.asan_dynamic-aarch64.dll!__asan::InitializeAsanInterceptors() Line 620
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_interceptors.cpp(620)
clang_rt.asan_dynamic-aarch64.dll!__asan::AsanInitInternal() Line 428
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_rtl.cpp(428)
clang_rt.asan_dynamic-aarch64.dll!__asan::Allocator::Allocate(unsigned __int64 size, unsigned __int64 alignment, __sanitizer::BufferedStackTrace * stack, __asan::AllocType alloc_type, bool can_fill) Line 477
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_allocator.cpp(477)
clang_rt.asan_dynamic-aarch64.dll!__asan::Allocator::Calloc(unsigned __int64 nmemb, unsigned __int64 size, __sanitizer::BufferedStackTrace * stack) Line 736
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_allocator.cpp(736)
clang_rt.asan_dynamic-aarch64.dll!__asan::asan_calloc(unsigned __int64 nmemb, unsigned __int64 size, __sanitizer::BufferedStackTrace * stack) Line 957
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_allocator.cpp(957)
clang_rt.asan_dynamic-aarch64.dll!calloc(unsigned __int64 nmemb, unsigned __int64 size) Line 115
	at Z:\Work\Microblink\llvm-project\compiler-rt\lib\asan\asan_malloc_win.cpp(115)
[Inline Frame] clang_rt.asan_dynamic-aarch64.dll!internal_get_ptd_head() Line 243
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\per_thread_data.cpp(243)
[Inline Frame] clang_rt.asan_dynamic-aarch64.dll!internal_getptd_noexit() Line 268
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\per_thread_data.cpp(268)
clang_rt.asan_dynamic-aarch64.dll!__acrt_getptd_noexit() Line 279
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\per_thread_data.cpp(279)
clang_rt.asan_dynamic-aarch64.dll!__acrt_initialize_ptd() Line 35
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\per_thread_data.cpp(35)
clang_rt.asan_dynamic-aarch64.dll!__acrt_execute_initializers(const __acrt_initializer * first, const __acrt_initializer * last) Line 25
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\shared_initialization.cpp(25)
clang_rt.asan_dynamic-aarch64.dll!__acrt_initialize() Line 287
	at C:\Users\dodo\Desktop\builds\llvm-build\minkernel\crts\ucrt\src\appcrt\internal\initialization.cpp(287)
clang_rt.asan_dynamic-aarch64.dll!__scrt_initialize_crt(__scrt_module_type module_type) Line 199
	at d:\a01\_work\16\s\src\vctools\crt\vcstartup\src\utility\utility.cpp(199)
clang_rt.asan_dynamic-aarch64.dll!dllmain_crt_process_attach(HINSTANCE__ * const instance, void * const reserved) Line 35
	at d:\a01\_work\16\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp(35)
clang_rt.asan_dynamic-aarch64.dll!dllmain_crt_dispatch(HINSTANCE__ * const instance, const unsigned long reason, void * const reserved) Line 219
	at d:\a01\_work\16\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp(219)
clang_rt.asan_dynamic-aarch64.dll!dllmain_dispatch(HINSTANCE__ * const instance, const unsigned long reason, void * const reserved) Line 276
	at d:\a01\_work\16\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp(276)
clang_rt.asan_dynamic-aarch64.dll!_DllMainCRTStartup(HINSTANCE__ * const instance, const unsigned long reason, void * const reserved) Line 334
	at d:\a01\_work\16\s\src\vctools\crt\vcstartup\src\startup\dll_dllmain.cpp(334)
ntdll.dll!00007ffe363d95f4()
ntdll.dll!00007ffe36416108()
ntdll.dll!00007ffe36415ebc()
ntdll.dll!00007ffe36415eec()
ntdll.dll!00007ffe364998cc()
ntdll.dll!00007ffe3641f80c()
ntdll.dll!00007ffe3641f670()
ntdll.dll!00007ffe3641f5e0()
ntdll.dll!00007ffe3641f578()

[DoDoENT]

It appears that interception_win.cpp also needs to be patched - it always assumes Intel architecture within GetInstructionSize and then fails when executing ARM64. Can we safely assume that for RISC CPUs, such as ARM64, the result here should always be 8 (i.e. 64-bit)?

[vitalybuka]

Then this is a separate issue. Please send new patches.

In D116376#3237140, @DoDoENT wrote:

It appears that interception_win.cpp also needs to be patched - it always assumes Intel architecture within GetInstructionSize and then fails when executing ARM64. Can we safely assume that for RISC CPUs, such as ARM64, the result here should always be 8 (i.e. 64-bit)?

Sorry, I have no idea.

[DoDoENT]

In D116376#3242664, @vitalybuka wrote:

Then this is a separate issue. Please send new patches.

In D116376#3237140, @DoDoENT wrote:

It appears that interception_win.cpp also needs to be patched - it always assumes Intel architecture within GetInstructionSize and then fails when executing ARM64. Can we safely assume that for RISC CPUs, such as ARM64, the result here should always be 8 (i.e. 64-bit)?

Sorry, I have no idea.

OK, thank you for all the help and for merging the patch.

compiler-rt/lib/sanitizer_common/sanitizer_unwind_win.cpp

void BufferedStackTrace::UnwindSlow(uptr pc, void *context, u32 max_depth) {
   CHECK_GE(max_depth, 2);
   CONTEXT ctx = *(CONTEXT *)context;
   STACKFRAME64 stack_frame;
   memset(&stack_frame, 0, sizeof(stack_frame));
 
   InitializeDbgHelpIfNeeded();
 
   size = 0;
-#if defined(_WIN64)
+#    if SANITIZER_WINDOWS64
+#      if SANITIZER_ARM64
+  int machine_type = IMAGE_FILE_MACHINE_ARM64;
+  stack_frame.AddrPC.Offset = ctx.Pc;
+  stack_frame.AddrFrame.Offset = ctx.Fp;
+  stack_frame.AddrStack.Offset = ctx.Sp;
+#      else
   int machine_type = IMAGE_FILE_MACHINE_AMD64;
   stack_frame.AddrPC.Offset = ctx.Rip;
   stack_frame.AddrFrame.Offset = ctx.Rbp;
   stack_frame.AddrStack.Offset = ctx.Rsp;
-#else
+#      endif
+#    else
   int machine_type = IMAGE_FILE_MACHINE_I386;
   stack_frame.AddrPC.Offset = ctx.Eip;
   stack_frame.AddrFrame.Offset = ctx.Ebp;
   stack_frame.AddrStack.Offset = ctx.Esp;
-#endif
+#    endif
   stack_frame.AddrPC.Mode = AddrModeFlat;
   stack_frame.AddrFrame.Mode = AddrModeFlat;
   stack_frame.AddrStack.Mode = AddrModeFlat;
   while (StackWalk64(machine_type, GetCurrentProcess(), GetCurrentThread(),
                      &stack_frame, &ctx, NULL, SymFunctionTableAccess64,
                      SymGetModuleBase64, NULL) &&
          size < Min(max_depth, kStackTraceMax)) {
     trace_buffer[size++] = (uptr)stack_frame.AddrPC.Offset;
   }
 }
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
-#endif  // #if !SANITIZER_GO
+#    ifdef __clang__
+#      pragma clang diagnostic pop
+#    endif
+#  endif  // #if !SANITIZER_GO
 
 #endif  // SANITIZER_WINDOWS

compiler-rt/lib/sanitizer_common/sanitizer_win.cpp

bool SignalContext::IsStackOverflow() const {
   return (DWORD)GetType() == EXCEPTION_STACK_OVERFLOW;
 }
 
 void SignalContext::InitPcSpBp() {
   EXCEPTION_RECORD *exception_record = (EXCEPTION_RECORD *)siginfo;
   CONTEXT *context_record = (CONTEXT *)context;
 
   pc = (uptr)exception_record->ExceptionAddress;
-#ifdef _WIN64
+#  if SANITIZER_WINDOWS64
+#    if SANITIZER_ARM64
+  bp = (uptr)context_record->Fp;
+  sp = (uptr)context_record->Sp;
+#    else
   bp = (uptr)context_record->Rbp;
   sp = (uptr)context_record->Rsp;
-#else
+#    endif
+#  else
   bp = (uptr)context_record->Ebp;
   sp = (uptr)context_record->Esp;
-#endif
+#  endif
 }
 
 uptr SignalContext::GetAddress() const {
   EXCEPTION_RECORD *exception_record = (EXCEPTION_RECORD *)siginfo;
   if (exception_record->ExceptionCode == EXCEPTION_ACCESS_VIOLATION)
     return exception_record->ExceptionInformation[1];
   return (uptr)exception_record->ExceptionAddress;
 }
 
 bool SignalContext::IsMemoryAccess() const {
   return ((EXCEPTION_RECORD *)siginfo)->ExceptionCode ==