[compiler-rt] Simplify definition of uptr

[arichardson]

We can rely on the compiler-provided macro UINTPTR_TYPE for all
non-MSVC compilers. I verified via https://godbolt.org/z/MW9KMjv5f that
this works for MSVC as well as GCC 4.5 Clang 3.0, so that should cover
all supported compilers.

This means we no longer need to explicitly handle new architectures
and as an added bonus also adds support for architectures where
unsigned long long cannot be used to hold pointers (e.g. CHERI).

[llvmbot]

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

Author: Alexander Richardson (arichardson)

Changes

We can rely on the compiler-provided macro UINTPTR_TYPE for all
non-MSVC compilers. I verified via https://godbolt.org/z/MW9KMjv5f that
this works for MSVC as well as GCC 4.5 Clang 3.0, so that should cover
all supported compilers.

This means we no longer need to explicitly handle new architectures
and as an added bonus also adds support for architectures where
unsigned long long cannot be used to hold pointers (e.g. CHERI).

Fixes: #101998

---

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

1 Files Affected:

• (modified) compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h (+8-8)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 3af47c2e2ff7a7..f8f03454ea169a 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -138,19 +138,19 @@
 // in a portable way by the language itself.
 namespace __sanitizer {
 
-#if defined(_WIN64)
+#if defined(__UINTPTR_TYPE__)
+typedef __UINTPTR_TYPE__ uptr;
+typedef __INTPTR_TYPE__ sptr;
+#elif defined(_WIN64)
 // 64-bit Windows uses LLP64 data model.
 typedef unsigned long long uptr;
 typedef signed long long sptr;
-#else
-#  if (SANITIZER_WORDSIZE == 64) || SANITIZER_APPLE || SANITIZER_WINDOWS
-typedef unsigned long uptr;
-typedef signed long sptr;
-#  else
+#elif defined(_WIN32)
 typedef unsigned int uptr;
 typedef signed int sptr;
-#  endif
-#endif  // defined(_WIN64)
+#else
+#  error Unsupported compiler, missing __UINTPTR_TYPE__
+#endif  // defined(__UINTPTR_TYPE__)
 #if defined(__x86_64__)
 // Since x32 uses ILP32 data model in 64-bit hardware mode, we must use
 // 64-bit pointer to unwind stack frame.

[mstorsjo]

This change also breaks i686 mingw builds in the same way as #106135

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 redefinition with different types ('ULONG_PTR' (aka 'unsigned long') vs '__sanitizer::uptr' (aka 'unsigned int'))
  147 |   __MINGW_EXTENSION typedef ULONG_PTR SIZE_T,*PSIZE_T;
      |                                       ^
/home/martin/code/llvm-mingw/llvm-project/compiler-rt/lib/interception/interception.h:28:30: note: previous definition is here
   28 | typedef __sanitizer::uptr    SIZE_T;
      |                              ^
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:148:38: error: typedef redefinition with different types ('LONG_PTR' (aka 'long') vs '__sanitizer::sptr' (aka 'int'))
  148 |   __MINGW_EXTENSION typedef LONG_PTR SSIZE_T,*PSSIZE_T;
      |                                      ^
/home/martin/code/llvm-mingw/llvm-project/compiler-rt/lib/interception/interception.h:29:30: note: previous definition is here
   29 | typedef __sanitizer::sptr    SSIZE_T;
      |                              ^
2 errors generated.

[mstorsjo]

(Marking as request changes, to set the status for clarity.)

[arichardson]

(Marking as request changes, to set the status for clarity.)

I've rebased this on top of #106311 which should hopefully fix the Windows issues you were seeing.

[github-actions]

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

[arichardson]

@mstorsjo I confirmed that llvm-mingw ./build-compiler-rt.sh --build-sanitizers still works, okay to merge?

[mstorsjo]

I think this looks reasonable, and as you've tested that it works with llvm-mingw, I guess it works. I haven't tested building it with MSVC though.

[aeubanks]

this has uncovered some nasty macro redefinitions in Linux headers which affects Android asan builds: https://crbug.com/365812505. can we revert this for now?

[arichardson]

Oh that is rather nasty - it sounds like Clang has the wrong UINTPTR_TYPE for Android arm32? And I guess changing that could have surprising effects?

I am surprised you did not get a compiler error, the compiler-rt/lib/interception/interception_type_test.cpp file should have failed to compile (unless it happens that in that file asm/types.h is included first and redefines UINTPTR_TYPE.

I wonder if the workaround would be something like the following

#if defined(__arm) && defined(__linux__)
#include <asm/types.h>
#endif

Or alternatively just adding

#if defined(__arm) && defined(__linux__)
// Clang and the Linux Arm headers disagree on uintptr_t
typedef unsigned long uptr;
typedef unsigned long sptr;
#endif

[aeubanks]

Oh that is rather nasty - it sounds like Clang has the wrong UINTPTR_TYPE for Android arm32? And I guess changing that could have surprising effects?
IIUC clang agrees with gcc, so clang isn't wrong, it's just that the kernel wants a different type as __UINTPTR_TYPE__ for some reason I don't understand.

I am surprised you did not get a compiler error, the compiler-rt/lib/interception/interception_type_test.cpp file should have failed to compile (unless it happens that in that file asm/types.h is included first and redefines UINTPTR_TYPE.
I'm not sure of the details, but yes either include order, or whether or not the Linux header is included at all changes behavior

I wonder if the workaround would be something like the following

#if defined(__arm) && defined(__linux__)
#include <asm/types.h>
#endif

Or alternatively just adding

#if defined(__arm) && defined(__linux__)
// Clang and the Linux Arm headers disagree on uintptr_t
typedef unsigned long uptr;
typedef unsigned long sptr;
#endif

I'll send out a PR that hardcodes uptr on Linux Arm

[nico]

Is that still simpler than what we had before this PR then?

[arichardson]

Even if it's slightly more verbose, the advantage of the new definition is that uptr now precisely matches the type of uintptr_t rather than just being the same size.

[nico]

Except on Linux/arm, while previously things were fairly similar across platforms.