[libcxx-commits] [libcxx] [libc++] Properly implement array cookies in the ARM ABI (PR #160182)

[Nikolas Klauser via libcxx-commits]

================
@@ -41,13 +41,73 @@ template <class _Tp>
 struct __has_array_cookie : false_type {};
 #endif
 
+// Return the array cookie located before the given pointer.
+//
+// In the Itanium ABI
+// ------------------
+// The array cookie is stored immediately before the first element of the array. If the preferred alignment
+// of array elements (which is different from the ABI alignment) is more than that of size_t, additional
+// padding bytes exist before the array cookie. Assuming array elements of size and alignment 16 bytes, that
+// gives us the following layout:
+//
+// |ooooooooxxxxxxxxaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbccccccccccccccccdddddddddddddddd|
+//  ^^^^^^^^        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+//     |    ^^^^^^^^                               |
+//     |       |                              array elements
+//  padding    |
+//        array cookie
+//
+// In practice, it is sufficient to read the bytes immediately before the first array element.
+//
+//
+// In the ARM ABI
+// --------------
+// The array cookie is stored at the very start of the allocation and it has the following form:
+//
+//    struct array_cookie {
+//      std::size_t element_size; // element_size != 0
+//      std::size_t element_count;
+//    };
+//
+// Assuming elements of size and alignment 32 bytes, this gives us the following layout:
+//
+//  |xxxxxxxxXXXXXXXXooooooooooooooooaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb|
+//   ^^^^^^^^        ^^^^^^^^^^^^^^^^
+//      |    ^^^^^^^^        |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+// element size  |        padding                                 |
+//         element count                                     array elements
+//
+// We calculate the starting address of the allocation by taking into account the ABI (not the preferred)
+// alignment of the type.
 template <class _Tp>
 // Avoid failures when -fsanitize-address-poison-custom-array-cookie is enabled
 _LIBCPP_HIDE_FROM_ABI _LIBCPP_NO_SANITIZE("address") size_t __get_array_cookie(_Tp const* __ptr) {
   static_assert(
       __has_array_cookie<_Tp>::value, "Trying to access the array cookie of a type that is not guaranteed to have one");
-  size_t const* __cookie = reinterpret_cast<size_t const*>(__ptr) - 1; // TODO: Use a builtin instead
+
+#if defined(_LIBCPP_ABI_ITANIUM)
+
+  size_t const* __cookie = reinterpret_cast<size_t const*>(__ptr) - 1;
   return *__cookie;
+
+#elif defined(_LIBCPP_ABI_ARM)
+
+  struct _ArrayCookie {
+    size_t __element_size;
+    size_t __element_count;
+  };
+
+  size_t __cookie_size_with_padding = // max(sizeof(_ArrayCookie), alignof(T))
+      sizeof(_ArrayCookie) < alignof(_Tp) ? alignof(_Tp) : sizeof(_ArrayCookie);
+  char const* __allocation_start = reinterpret_cast<char const*>(__ptr) - __cookie_size_with_padding;
+  _ArrayCookie const* __cookie   = reinterpret_cast<_ArrayCookie const*>(__allocation_start);
+  return __cookie->__element_count;
----------------
philnik777 wrote:

> I think that's equivalent, yes. But that requires the use of these attributes, which I'm not convinced makes it clearer. I have a suggestion that IMO makes this clearer:
> 
> ```
> struct _ALIGNAS_TYPE(_Tp) _ArrayCookie {
>   size_t __element_size;
>   size_t __element_count;
> };
> 
> char const* __allocation_start = reinterpret_cast<char const*>(__ptr) - sizeof(_ArrayCookie);
> _ArrayCookie const* __cookie   = reinterpret_cast<_ArrayCookie const*>(__allocation_start);
> return __cookie->__element_count;
> ```
> 
> I like this because it simplifies the calculation of the padding, but it keeps things simple w.r.t. aliasing by using a `char const*` like everybody does everywhere.

I don't think this addresses the aliasing problem. We're technically reading out of bounds, which makes this somewhat problematic already, but the array itself is known to be of T. Indexing into that with a non-T is definitely a strict-aliasing violation, no matter what type you're using to offset the pointer. If you want to do this without non-standard attributes you'll have to use a `memcpy`. I'm also not convinced your version is clearer, since you have to do significantly more pointer magic.

> **Edit**: Turns out using the `__aligned__` attribute complains when the alignment of `_Tp` is less than that of the array cookie:
> 
> ```
> error: requested alignment is less than minimum alignment of 8 for type '_ArrayCookie'
> ```

This is only a problem with `alignas`. `[[gnu::aligned]]` works just fine.


https://github.com/llvm/llvm-project/pull/160182