-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
[compiler-rt] Work around incompatible Windows definitions of (S)SIZE_T #106311
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this seems to work fine now. (I don't have a strong opinion on whether this is a good way of solving it or not though.)
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_t
usesunsigned int
whereas
SIZE_T
isunsigned long
. The current definition results inuptr
not matchinguintptr_t
since 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.