[compiler-rt] ARM64 instruction are always 4 bytes long but GetInstructionSize in interception_win.cpp assumes x86_64 which has mixed sizes

[farzon]

Summary

Tests don't build for this target. Testing was done on chromium builds.
Fix is for: https://github.com/llvm/llvm-project/issues/64319
Before the changeclang_rt.asan_dynamic-aarch64.dll would crash at:
OverrideFunction -> OverrideFunctionWithHotPatch -> GetInstructionSize:825

After the change:
dllthunkintercept -> dllthunkgetrealaddressordie -> InternalGetProcAddress

[DavidSpickett]

Adding Omair here who is doing Windows on Arm work at Linaro right now.

Thanks for the fix! Please include context next time/in your next update https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. Phabricator works on diffs alone, so the context gives us visibility around the change.

Someone will ask about testing, and that someone is me, but last I remember the sanitizers were not stable enough to run on Linaro's Windows on Arm bots. Sounds like you are fixing that situation though so thanks for your work here, enabling them on the buildbots is a future option.

[DavidSpickett]

Also please put a tag at the start of the commit title, in this case it would be [compiler-rt] ARM64 instruction are always 4 bytes long but GetInstructionSize in interception_win.cpp assumes x86_64 which has mixed sizes. Helps us filter changes when looking at logs.

[omjavaid]

@farzon thanks for this fix. Please upload a full context diff using

git diff -U999999 --no-color HEAD^

Also kindly update commit message (summary section in this review) with following information:
What changes after your fix? For example list the sanitizer tests that pass after your fix.
Also include which test platform was used for the build.

[farzon]

Hi omjavaid, Unfortunately I can't build compilerRT tests on arm64, so my testing has. been on usage of clang_rt.asan_dynamic-aarch64.dll in chromium builds.

-DCOMPILER_RT_INCLUDE_TESTS

CMake Error at C:/Users/Farzon/project/llvm-project/llvm/cmake/modules/AddLLVM.cmake:1940 (add_dependencies):

The dependency target "cfi" of target "check-compiler-rt" does not exist.

Call Stack (most recent call first):

C:/Users/Farzon/project/llvm-project/llvm/cmake/modules/AddLLVM.cmake:1980 (add_lit_target)
test/CMakeLists.txt:114 (umbrella_lit_testsuite_end)

CMake Error at C:/Users/Farzon/project/llvm-project/llvm/cmake/modules/AddLLVM.cmake:1940 (add_dependencies):

The dependency target "cfi" of target "check-cfi" does not exist.

Call Stack (most recent call first):

C:/Users/Farzon/project/llvm-project/llvm/cmake/modules/AddLLVM.cmake:2006 (add_lit_target)
test/cfi/CMakeLists.txt:90 (add_lit_testsuite)

CMake Error at C:/Users/Farzon/project/llvm-project/llvm/cmake/modules/AddLLVM.cmake:1940 (add_dependencies):

The dependency target "cfi" of target "check-cfi-and-supported" does not
exist.

Call Stack (most recent call first):

test/cfi/CMakeLists.txt:94 (add_lit_target)

Before my change the app would abort with this callstack: OverrideFunction -> OverrideFunctionWithHotPatch -> GetInstructionSize:825
Now we get much further

Failing at InternalGetProcAddress for __asan_wrap_strcpy

[farzon]

https://github.com/llvm/llvm-project/commit/5a48a824aa0cf383cc8201d04c0a37ab45912a8c

compiler-rt/lib/interception/interception_win.cpp

 static const u8 kPrologueWithShortJump2[] = {
     0x4c, 0x8b, 0xc1, 0x8a, 0x01, 0x48, 0xff, 0xc1,
     0x84, 0xc0, 0x75, 0xf7,
 };
 #endif
 
 // Returns 0 on error.
 static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
+#if SANITIZER_ARM64
+  // An ARM64 instruction is 4 bytes long.
+  return 4;
+#endif
+
 #if SANITIZER_WINDOWS64
   if (memcmp((u8*)address, kPrologueWithShortJump1,
              sizeof(kPrologueWithShortJump1)) == 0 ||
       memcmp((u8*)address, kPrologueWithShortJump2,
              sizeof(kPrologueWithShortJump2)) == 0) {
     return 0;
   }
 #endif