[PATCH] D74364: [GWP-ASan] Update alignment on Android.

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:19
     "the page boundary? By default (false), we round up allocation size to the "
-    "nearest power of two (1, 2, 4, 8, 16) up to a maximum of 16-byte "
-    "alignment for performance reasons. Setting this to true can find single "
-    "byte buffer-overflows for multibyte allocations at the cost of "
-    "performance, and may be incompatible with some architectures.")
+    "nearest power of two (1, 2, 4, 8) up to a maximum of 8-byte alignment for "
+    "performance reasons. On Android, we use 8-byte alignment by "
----------------
I'm not positive if its safe to go down to 8 bytes here.  Until [recently](http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2293.htm), there was an ambiguity in the standard that meant most mallocs used 16-byte alignment.  It's gone now, but there may still be Android use cases that expect `malloc(16+)` to have the higher alignment (pretty sure this exists in Google3).

In TCMalloc, all `operator new`s get 8-byte alignment by default while `malloc` and friends follow the `memalign(16, size)` path.  But  I don't know how Bionic does this...


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:21
+    "performance reasons. On Android, we use 8-byte alignment by "
+    "default.Setting this to true can find single byte buffer-overflows for "
+    "multibyte allocations at the cost of performance, and may be incompatible "
----------------
Add space after `.`


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:14
+
+#ifdef __BIONIC__
 #include <stdlib.h>
----------------
Is this Bionic-specific, or Android generally?


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:40
+
+size_t alignEight(size_t RealAllocationSize) {
+  if (RealAllocationSize % 8 == 0)
----------------
static


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:46
+
+size_t alignPowerOfTwo(size_t RealAllocationSize) {
+  if (RealAllocationSize <= 2)
----------------
static


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:54
+
+#ifdef __BIONIC__
+static constexpr AlignmentStrategy PlatformDefaultAlignment =
----------------
Should this be the same for Android generally?


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/utilities_posix.cpp:56
+static constexpr AlignmentStrategy PlatformDefaultAlignment =
+    AlignmentStrategy::EIGHT;
+#else  // __BIONIC__
----------------
I think we should have a comment here explaining why rounding to 8 is sufficient for Android.


================
Comment at: compiler-rt/lib/gwp_asan/tests/alignment.cpp:29
+      {1, 8},   {2, 8},   {3, 8},       {4, 8},       {5, 8},   {7, 8},
+      {8, 8},   {9, 16},  {15, 16},     {16, 16},     {17, 24}, {31, 32},
+      {32, 32}, {33, 40}, {4095, 4096}, {4096, 4096},
----------------
The test is called `AlignToEight`, but we expect alignments of 16, 24, etc.... Maybe call it `AndroidAlignment` and just use 8 or 16, since that's what we need.

And maybe add a comment explaining Android's alignment scheme.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74364/new/

https://reviews.llvm.org/D74364