[compiler-rt] Work around incompatible Windows definitions of (S)SIZE_T #106311
Conversation
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Alexander Richardson (arichardson) ChangesThe interceptor types are supposed to match size_t (and the non-Windows It would probably be cleaner to stop using these uppercase types, but This also reverts commit 18e06e3. Full diff: https://github.com/llvm/llvm-project/pull/106311.diff 3 Files Affected:
diff --git a/compiler-rt/lib/interception/interception.h b/compiler-rt/lib/interception/interception.h
index 38c152952e3232..c061752231c64e 100644
--- a/compiler-rt/lib/interception/interception.h
+++ b/compiler-rt/lib/interception/interception.h
@@ -25,8 +25,14 @@
// These typedefs should be used only in the interceptor definitions to replace
// the standard system types (e.g. SSIZE_T instead of ssize_t)
-typedef __sanitizer::uptr SIZE_T;
-typedef __sanitizer::sptr SSIZE_T;
+// On Windows the system headers (basetsd.h) provide a conflicting definition
+// of SIZE_T/SSIZE_T that do not match the real size_t/ssize_t for 32-bit
+// systems (using long instead of the expected int). Work around the typedef
+// redefinition by #defining SIZE_T instead of using a typedef.
+// TODO: We should be using __sanitizer::usize (and a new ssize) instead of
+// these new macros as long as we ensure they match the real system definitions.
+#define SIZE_T __sanitizer::usize
+#define SSIZE_T __sanitizer::sptr
typedef __sanitizer::sptr PTRDIFF_T;
typedef __sanitizer::s64 INTMAX_T;
typedef __sanitizer::u64 UINTMAX_T;
diff --git a/compiler-rt/lib/interception/interception_type_test.cpp b/compiler-rt/lib/interception/interception_type_test.cpp
index 7c3de82a1e869c..ccf555c7b1bc20 100644
--- a/compiler-rt/lib/interception/interception_type_test.cpp
+++ b/compiler-rt/lib/interception/interception_type_test.cpp
@@ -12,17 +12,26 @@
//===----------------------------------------------------------------------===//
#include "interception.h"
+#include "sanitizer_common/sanitizer_type_traits.h"
-#if SANITIZER_LINUX || SANITIZER_APPLE
-
+#if __has_include(<sys/types.h>)
#include <sys/types.h>
+#endif
#include <stddef.h>
#include <stdint.h>
-COMPILER_CHECK(sizeof(::SIZE_T) == sizeof(size_t));
-COMPILER_CHECK(sizeof(::SSIZE_T) == sizeof(ssize_t));
-COMPILER_CHECK(sizeof(::PTRDIFF_T) == sizeof(ptrdiff_t));
+COMPILER_CHECK((__sanitizer::is_same<__sanitizer::uptr, ::uintptr_t>::value));
+COMPILER_CHECK((__sanitizer::is_same<__sanitizer::sptr, ::intptr_t>::value));
+COMPILER_CHECK((__sanitizer::is_same<__sanitizer::usize, ::size_t>::value));
+COMPILER_CHECK((__sanitizer::is_same<::PTRDIFF_T, ::ptrdiff_t>::value));
+COMPILER_CHECK((__sanitizer::is_same<::SIZE_T, ::size_t>::value));
+#if !SANITIZER_WINDOWS
+// No ssize_t on Windows.
+COMPILER_CHECK((__sanitizer::is_same<::SSIZE_T, ::ssize_t>::value));
+#endif
+// TODO: These are not actually the same type on Linux (long vs long long)
COMPILER_CHECK(sizeof(::INTMAX_T) == sizeof(intmax_t));
+COMPILER_CHECK(sizeof(::UINTMAX_T) == sizeof(uintmax_t));
# if SANITIZER_GLIBC || SANITIZER_ANDROID
COMPILER_CHECK(sizeof(::OFF64_T) == sizeof(off64_t));
@@ -36,4 +45,3 @@ COMPILER_CHECK(sizeof(::OFF64_T) == sizeof(off64_t));
COMPILER_CHECK(sizeof(::OFF_T) == sizeof(off_t));
# endif
-#endif
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 1607436e476e52..26089948c3e2d0 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -143,7 +143,7 @@ namespace __sanitizer {
typedef unsigned long long uptr;
typedef signed long long sptr;
#else
-# if (SANITIZER_WORDSIZE == 64) || SANITIZER_APPLE || SANITIZER_WINDOWS
+# if (SANITIZER_WORDSIZE == 64) || SANITIZER_APPLE
typedef unsigned long uptr;
typedef signed long sptr;
# else
|
Created using spr 1.3.6-beta.1
|
|
||
| #if SANITIZER_LINUX || SANITIZER_APPLE |
There was a problem hiding this comment.
This change is unrelated, but IMO we should check these static asserts everywhere possible.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.6-beta.1
There was a problem hiding this comment.
Unfortunately, this doesn't work.
From the mingw build:
In file included from /home/martin/code/llvm-mingw/llvm-project/compiler-rt/lib/interception/interception_win.cpp:132:
In file included from /home/martin/clang-trunk/i686-w64-mingw32/include/windows.h:69:
In file included from /home/martin/clang-trunk/i686-w64-mingw32/include/windef.h:9:
In file included from /home/martin/clang-trunk/i686-w64-mingw32/include/minwindef.h:163:
In file included from /home/martin/clang-trunk/i686-w64-mingw32/include/winnt.h:150:
/home/martin/clang-trunk/i686-w64-mingw32/include/basetsd.h:147:39: error: typedef declarator cannot be qualified
147 | __MINGW_EXTENSION typedef ULONG_PTR SIZE_T,*PSIZE_T;
| ^~~~~~
/home/martin/code/llvm-mingw/llvm-project/compiler-rt/lib/interception/interception.h:34:29: note: expanded from macro 'SIZE_T'
34 | #define SIZE_T __sanitizer::usize
| ~~~~~~~~~~~~~^
From the MSVC build:
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(195): error C2665: '__interception::OverrideFunction': no overloaded function could convert all the argument types
C:\code\llvm-project\compiler-rt\lib\interception\interception_win.h(31): note: could be 'bool __interception::OverrideFunction(const char *,__interception::uptr,__interception::uptr *)'
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(195): note: 'bool __interception::OverrideFunction(const char *,__interception::uptr,__interception::uptr *)': cannot convert argument 3 from '__sanitizer::uptr *' to '__interception::uptr *'
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(197): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast
C:\code\llvm-project\compiler-rt\lib\interception\interception_win.h(28): note: or 'bool __interception::OverrideFunction(__interception::uptr,__interception::uptr,__interception::uptr *)'
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(195): note: 'bool __interception::OverrideFunction(__interception::uptr,__interception::uptr,__interception::uptr *)': cannot convert argument 1 from 'const char [15]' to '__interception::uptr'
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(195): note: There is no context in which this conversion is possible
C:\code\llvm-project\compiler-rt\lib\asan\asan_win.cpp(195): note: while trying to match the argument list '(const char [15], __sanitizer::uptr, __sanitizer::uptr *)'
Thanks for testing this - I will try to rework it to fix the problem. |
Created using spr 1.3.6-beta.1
I tried setting up a Windows VM, but was not able to get this working today. However, I managed to reproduce these build failures with mingw packages and: I can't successfully build the libraries since I get linker errors, but I've confirmed that the latest version of this patch fixes the compilation errors reported by @mstorsjo |
Created using spr 1.3.6-beta.1
Indeed, this version does build successfully with MSVC, and it does get past the earlier quoted errors when building for mingw targets, but for mingw x86_64 (and MSVC x86_64 too), it fails on other issues due to some of the other, new static asserts you've added here: FWIW, it should be quite easy to test the build for mingw targets, with llvm-mingw. To do that:
The compiler-rt sanitizers are buildable for armv7 and aarch64 too, but the |
Created using spr 1.3.6-beta.1
Thank you very much for those instructions! I have now built for Win64 and avoided the static_assert. I plan to clean up off_t handling in a separate PR. EDIT: also checked armv7 and aarch64 and both of those also compile. So hopefully this is ready to merge. |
arichardson
deleted the
users/arichardson/spr/compiler-rt-work-around-incompatible-windows-definitions-of-ssize_t
branch
|
We're seeing a build failures on Mac LLVM compile like: I'm trying to reproduce it locally - but until I do and figure out why exactly it is happening in our configuration, would you consider reverting? Thanks |
It sounds like on macos 32 bit ptrdiff_t is int rather than long. You could replace Can you tell me which target you are building for? |
|
The other option would be |
|
My clang command line has Let me experiment with your suggestions, thanks! |
The interceptor types are supposed to match size_t (and the non-Windows
ssize_t) exactly, but on 32-bit Windows
size_tusesunsigned intwhereas
SIZE_Tisunsigned long. The current definition results inuptrnot matchinguintptr_tsince we otherwise get typedefredefinition errors. Work around this by using a #define instead of
a typedef when defining SIZE_T.
It would probably be cleaner to stop using these uppercase types, but
that is a rather invasive change and this one is the minimal change to
allow uptr to match uintptr_t on Windows.
To ensure this compiles on Windows, we also remove the interceptor.h
defines of uptr (that do not always match __sanitizer::uptr) and rely
on __sanitizer::uptr instead. The interceptor types most likely predate
those other types so clean up the unnecessary definition while here.
This also reverts commit 18e06e3 and
commit bb27dd8.