Skip to content

[compiler-rt] Simplify and rename of operator_new_size_type #83912

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

Conversation

arichardson
Copy link
Member

We can rely on the compiler-provided macro SIZE_TYPE for all
non-MSVC compilers and fall back to uptr otherwise.
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 supported compilers.

While touching this also rename operator_new_size_type to usize which
makes it more obvious that this is the equivalent to size_t within
the sanitizers runtime (which I plan to use in follow-up changes).

Created using spr 1.3.6-beta.1
@llvmbot
Copy link
Member

llvmbot commented Mar 4, 2024

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

Author: Alexander Richardson (arichardson)

Changes

We can rely on the compiler-provided macro SIZE_TYPE for all
non-MSVC compilers and fall back to uptr otherwise.
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 supported compilers.

While touching this also rename operator_new_size_type to usize which
makes it more obvious that this is the equivalent to size_t within
the sanitizers runtime (which I plan to use in follow-up changes).


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

3 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_common.h (+1-1)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h (+3-8)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_placement_new.h (+1-1)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.h b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
index 47697ef280aa0d..c451fc962c5294 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
@@ -1098,7 +1098,7 @@ inline u32 GetNumberOfCPUsCached() {
 
 }  // namespace __sanitizer
 
-inline void *operator new(__sanitizer::operator_new_size_type size,
+inline void *operator new(__sanitizer::usize size,
                           __sanitizer::LowLevelAllocator &alloc) {
   return alloc.Allocate(size);
 }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 992721757e88da..294e330c4d5611 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -191,15 +191,10 @@ typedef uptr OFF_T;
 #endif
 typedef u64  OFF64_T;
 
-#if (SANITIZER_WORDSIZE == 64) || SANITIZER_APPLE
-typedef uptr operator_new_size_type;
+#ifdef __SIZE_TYPE__
+typedef __SIZE_TYPE__ usize;
 #else
-# if defined(__s390__) && !defined(__s390x__)
-// Special case: 31-bit s390 has unsigned long as size_t.
-typedef unsigned long operator_new_size_type;
-# else
-typedef u32 operator_new_size_type;
-# endif
+typedef uptr usize;
 #endif
 
 typedef u64 tid_t;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_placement_new.h b/compiler-rt/lib/sanitizer_common/sanitizer_placement_new.h
index 1ceb8b909268f3..11ebcae6b3f912 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_placement_new.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_placement_new.h
@@ -17,7 +17,7 @@
 
 #include "sanitizer_internal_defs.h"
 
-inline void *operator new(__sanitizer::operator_new_size_type sz, void *p) {
+inline void *operator new(__sanitizer::usize sz, void *p) {
   return p;
 }
 

Copy link

github-actions bot commented Mar 4, 2024

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

Created using spr 1.3.6-beta.1
@arichardson arichardson requested review from vitalybuka and pcc March 4, 2024 22:22
arichardson added a commit to arichardson/upstream-llvm-project that referenced this pull request Mar 7, 2024
We can rely on the compiler-provided macro __SIZE_TYPE__ for all
non-MSVC compilers and fall back to `uptr` otherwise.
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 supported compilers.

While touching this also rename operator_new_size_type to usize which
makes it more obvious that this is the equivalent to size_t within
the sanitizers runtime (which I plan to use in follow-up changes).

Pull Request: llvm#83912
# else
typedef u32 operator_new_size_type;
# endif
typedef uptr usize;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it work for s390?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's correct: https://godbolt.org/z/rjnhWeW6v

@arichardson arichardson merged commit c58c827 into main Mar 9, 2024
@arichardson arichardson deleted the users/arichardson/spr/compiler-rt-simplify-and-rename-of-operator_new_size_type branch March 9, 2024 05:22
@zmodem
Copy link
Collaborator

zmodem commented Aug 26, 2024

This broke building with MSVC for 32-bit x86: #101998. Can we revert it? It still seems to revert cleanly, and I can't really think of a better fix than putting the old operator_new_size_type back.

@arichardson
Copy link
Member Author

This broke building with MSVC for 32-bit x86: #101998. Can we revert it? It still seems to revert cleanly, and I can't really think of a better fix than putting the old operator_new_size_type back.

What is size_t on 32-bit windows? Is it unsigned long or unsigned int Surprised it's not the same as uptr? I will upload a patch shortly that should hopefully fix MSVC.

@zmodem
Copy link
Collaborator

zmodem commented Aug 26, 2024

What is size_t on 32-bit windows?

It seems to be unsigned int.

@arichardson
Copy link
Member Author

arichardson commented Aug 26, 2024

What is size_t on 32-bit windows?

It seems to be unsigned int.

I just looked at this in godbolt and it seems like it should work as intended on MSVC 32-bit: https://godbolt.org/z/3KT59qKoh Do you build with any special flags that could affect this?

@arichardson
Copy link
Member Author

arichardson commented Aug 26, 2024

It looks like the problem is

# if (SANITIZER_WORDSIZE == 64) || SANITIZER_APPLE || SANITIZER_WINDOWS, this should not be checking SANITIZER_WINDOWS, since _WIN32 uses unsigned int not unsigned long. Will upload a PR. EDIT: #83912 and #106155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants