[Sanitizer] Clean up SANITIZER_CAN_USE_ALLOCATOR64 logic

[thetruestblue]

Summary

Update: A change to this code recently broke our bots after this change enabled the 64 bit allocator for defined(aarch64): https://reviews.llvm.org/D137136

We added logic initially to get our test passing, but want to further clean up this code to enable MacOS to use allocator64 and increase readability and clarity of the logic.

rdar://103647896

[wrotki]

LGTM

[wrotki]

Before you land this - I just noticed that your #elifs spacing is slightly off.

[thetruestblue]

In D141171#4036669, @wrotki wrote:

Before you land this - I just noticed that your #elifs spacing is slightly off.

Yep. Will update the patch shortly. Waiting for feedback from @rsundahl and @yln since we have talked about this previously and were undecided on the best path forward.

[rsundahl]

In the absence of any justification for using the 32 bit allocator, I agree that we should be using the 64 bit version. This LGTM.

[yln]

I think we can drop && !defined(SANITIZER_APPLE) altogether and let the "default" (last else) case handle things. It definitely doesn't make sense in it's current form: && takes precedence so the code is currently saying: xxx || (hexagon && !apple)

~~~ Optional change ~~~
If we want to go a step further and be good stewards of this code, then we could even try to cleanup these #if-else. My "leave the campground cleaner than you found it" persona would like to do this, but from experience I know that we probably don't get it completely right in the first try.

I would imagine an attempt to do this like follows. First listing all the "exceptions" and then doing the "default", removing all current cases that could be covered by the default.

1. "Always on" platforms (irrespective of arch)
2. "Always off" platforms (irrespective or arch)
3. 64-bit archs that still want to use the 32-bit allocator (not sure if hexagon is 32 or 64 bit)
4. (the other way around is empty)
5. Default case (remove all mentions of things that are covered by this)

#ifndef SANITIZER_CAN_USE_ALLOCATOR64
#  if SANITIZER_FUCHSIA
#    define SANITIZER_CAN_USE_ALLOCATOR64 1
#  elif SANITIZER_RISCV64 || SANITIZER_IOS
#    define SANITIZER_CAN_USE_ALLOCATOR64 0
#  elif defined(__mips64) || defined(__hexagon__)
#    define SANITIZER_CAN_USE_ALLOCATOR64 0
#  else
#    define SANITIZER_CAN_USE_ALLOCATOR64 (SANITIZER_WORDSIZE == 64)
#  endif
#endif

[thetruestblue]

In D141171#4038142, @yln wrote:

I think we can drop && !defined(SANITIZER_APPLE) altogether and let the "default" (last else) case handle things. It definitely doesn't make sense in it's current form: && takes precedence so the code is currently saying: xxx || (hexagon && !apple)

We need !SANITIZER_APPLE [we don't need !defined(SANITIZER_APPLE) tho, the way written here is incorrect]
Because otherwise MacOS takes this path because arm is defined. But you're right. I moved it to the end thinking it would make readability easier and spaced on the precedence.

Updated the diff for now with a fix for the spacing and that logic. But need to think on your optional suggestion, cause I agree it needs a refactor. Did you intentionally skip i386?

[thetruestblue]

In D141171#4038389, @thetruestblue wrote:

In D141171#4038142, @yln wrote:

I think we can drop && !defined(SANITIZER_APPLE) altogether and let the "default" (last else) case handle things. It definitely doesn't make sense in it's current form: && takes precedence so the code is currently saying: xxx || (hexagon && !apple)

We need !SANITIZER_APPLE [we don't need !defined(SANITIZER_APPLE) tho, the way written here is incorrect]
Because otherwise MacOS takes this path because arm is defined. But you're right. I moved it to the end thinking it would make readability easier and spaced on the precedence.

Updated the diff for now with a fix for the spacing and that logic. But need to think on your optional suggestion, cause I agree it needs a refactor. Did you intentionally skip i386?

Disregard. You're absolutely correct @yln. This negation is unnecessary.

[thetruestblue]

Removed check for apple in second conditional.

[yln]

If we are not doing the optional cleanup, then I think I would prefer this:

#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA
#    define SANITIZER_CAN_USE_ALLOCATOR64 1
# elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \
      SANITIZER_RISCV64 || defined(__hexagon__) || SANITIZER_IOS
#    define SANITIZER_CAN_USE_ALLOCATOR64 0
#  else

That is, adding SANITIZER_IOS to the branch that disable the 64 bit allocator.

Please also adjust the outdated comment:

// But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
-->
// But in some cases (e.g. iOS/AArch64's 39-bit address space) SizeClassAllocator64

[thetruestblue]

In D141171#4040524, @yln wrote:

If we are not doing the optional cleanup, then I think I would prefer this:

#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA
#    define SANITIZER_CAN_USE_ALLOCATOR64 1
# elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \
      SANITIZER_RISCV64 || defined(__hexagon__) || SANITIZER_IOS
#    define SANITIZER_CAN_USE_ALLOCATOR64 0
#  else

That is, adding SANITIZER_IOS to the branch that disable the 64 bit allocator.

Please also adjust the outdated comment:

// But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
-->
// But in some cases (e.g. iOS/AArch64's 39-bit address space) SizeClassAllocator64

I prefer the optional cleanup. I will update.

[thetruestblue]

incorporate @yln's suggestion.

[thetruestblue]

update revision

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

286–287

@phosek isn't SANITIZER_FUCHSIA is SANITIZER_WORDSIZE == 64, as it's 64bit only?

[phosek]

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

286–287

Yes, I think it should be safe to remove this altogether and rely on the generic #else branch.

[yln]

Thanks Blue!

I think we can try to incorporate Vitaly's & and Petr's comment in our first attempt to land this. *fingers crossed*
Please monitor this revision and be ready to revert in case this creates fallout.

[rsundahl]

This looks a lot better. Thanks @thetruestblue !

compiler-rt/lib/sanitizer_common/sanitizer_platform.h

 
 #if defined(__loongarch_lp64)
 #  define SANITIZER_LOONGARCH64 1
 #else
 #  define SANITIZER_LOONGARCH64 0
 #endif
 
 // By default we allow to use SizeClassAllocator64 on 64-bit platform.
-// But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
-// does not work well and we need to fallback to SizeClassAllocator32.
+// But in some cases SizeClassAllocator64 does not work well and we need to
+// fallback to SizeClassAllocator32.
 // For such platforms build this code with -DSANITIZER_CAN_USE_ALLOCATOR64=0 or
 // change the definition of SANITIZER_CAN_USE_ALLOCATOR64 here.
 #ifndef SANITIZER_CAN_USE_ALLOCATOR64
-#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA
-#    define SANITIZER_CAN_USE_ALLOCATOR64 1
-#  elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \
-      SANITIZER_RISCV64 || defined(__hexagon__) || \
-      (SANITIZER_APPLE && defined(__aarch64__))
+#  if SANITIZER_RISCV64 || SANITIZER_IOS
+#    define SANITIZER_CAN_USE_ALLOCATOR64 0
+#  elif defined(__mips64) || defined(__hexagon__)
 #    define SANITIZER_CAN_USE_ALLOCATOR64 0
 #  else
 #    define SANITIZER_CAN_USE_ALLOCATOR64 (SANITIZER_WORDSIZE == 64)
 #  endif
 #endif
 
 // The range of addresses which can be returned my mmap.
 // FIXME: this value should be different on different platforms.  Larger values