[scudo] Modify hard-coded page size for Android.

[cferris]

Summary

On Android, if PAGE_SIZE is defined, use that as the hard-coded value.
Otherwise, fallback to using getting the page size.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/common.h

134–138

How about simply return PAGE_SIZE if it's defined?

This may convey an information that PAGE_SIZE is a way to customize the page size value.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/common.h

134–138

Or is it supposed to be

#if defined(SCUDO_ANDROID)
// Bionic uses a hardcoded value if PAGE_SIZE is defined.
#if defined(PAGE_SIZE)
  return PAGE_SIZE;
#endif
#endif // if defined(SCUDO_ANDROID)

[enh]

compiler-rt/lib/scudo/standalone/common.h

134–138

Or is it supposed to be

#if defined(SCUDO_ANDROID)
// Bionic uses a hardcoded value if PAGE_SIZE is defined.
#if defined(PAGE_SIZE)
  return PAGE_SIZE;
#endif
#endif // if defined(SCUDO_ANDROID)

well, it's the same thing, right?

the reason i didn't suggest dropping the SCUDO_ANDROID part and only checking PAGE_SIZE is that that would be a behavior change for other libcs. (basically: glibc/musl on x86/x86-64/alpha/s390.) which i _think_ is fine anyway (because afaik we're the only libc left that defines PAGE_SIZE for an architecture where the page size isn't actually a constant), but since that would strictly be a behavioral change...

but, yeah, i think there's a decent argument for just having

#if defined(PAGE_SIZE) 
  return PAGE_SIZE;
#endif

if you want.

[Chia-hungDuan]

compiler-rt/lib/scudo/standalone/common.h

134–138

Yep, they are the same in the result and I agree with your concern on the behavioral change.

To me, the difference is the implication of the use of PAGE_SIZE. Nested under SCUDO_ANDROID will imply it's only used on Android. In contrast, it'll give me the impression that everyone can use it to define constant page size (however, it only works on Android).

That's why I think nesting PAGE_SIZE under SCUDO_ANDROID will be better. Not a strong opinion, feel free to keep the current structure!

[enh]

compiler-rt/lib/scudo/standalone/common.h

134–138

yeah, i don't have a strong opinion either, but given that PAGE_SIZE is just a regular libc #define [at least traditionally], it seems weird using that as a general configuration option.

maybe a comment like // Most Android builds have a build-time constant page size. would have helped?

[cferris]

Added include of unistd.h and modified the return given comments.

[cferris]

I modified the change and added an include because without unistd.h, PAGE_SIZE is not defined in many cases.

[cferris]

compiler-rt/lib/scudo/standalone/common.h

134–138

Changed this to a slightly different version, but the spirit of this suggestion.

compiler-rt/lib/scudo/standalone/common.h

 #include "internal_defs.h"
 
 #include "fuchsia.h"
 #include "linux.h"
 #include "trusty.h"
 
 #include <stddef.h>
 #include <string.h>
+#include <unistd.h>
 
 namespace scudo {
 
 template <class Dest, class Source> inline Dest bit_cast(const Source &S) {
   static_assert(sizeof(Dest) == sizeof(Source), "");
   Dest D;
   memcpy(&D, &S, sizeof(D));
   return D;

 #endif
   __asm__ __volatile__("" ::: "memory");
 }
 
 // Platform specific functions.
 
 extern uptr PageSizeCached;
 uptr getPageSizeSlow();
 inline uptr getPageSizeCached() {
-  // Bionic uses a hardcoded value.
-  if (SCUDO_ANDROID)
-    return 4096U;
+#if SCUDO_ANDROID && defined(PAGE_SIZE)
+  // Most Android builds have a build-time constant page size.
+  return PAGESIZE;
+#endif
   if (LIKELY(PageSizeCached))
     return PageSizeCached;
   return getPageSizeSlow();
 }
 
 // Returns 0 if the number of CPUs could not be determined.
 u32 getNumberOfCPUs();