[libc++][AIX] Make basic_string layout compatible with earlier version

[xingxue]

Summary

Patch D123580 changed to use bit fields for strings in long and short mode. As a result, this changes the layout of these strings on AIX because bit fields on AIX are 4 bytes, which breaks the ABI compatibility with earlier strings before the change on AIX. This patch applies the fix proposed by @hubert.reinterpretcast in https://reviews.llvm.org/D127672#3590741 to make string layout compatible. This patch will also make test cases alignof.compile.pass.cpp and sizeof.compile.pass.cpp introduced in D127672 (later reverted) pass on AIX.

[philnik]

libcxx/include/string

724

How exactly is this different from using __atrribute__((__packed__))? If it's not I would suggest to put __is_long_ and __cap_ into it's own struct and mark only that as __attribute__((__packed__))to avoid the explicit padding and alignas() specifiers.

[hubert.reinterpretcast]

libcxx/include/string

724

That should work. Any preference between base class, anonymous struct, or named member?

The named member version is the most pragmatic (does not change whether the type is standard layout, does not use language extension).

[philnik]

libcxx/include/string

724

I think an anonymous struct would be the best option. That makes it almost identical to the current version, so there wouldn't have to be any code changes elsewhere.

[xingxue]

Changed to use anonymous struct and __attribute__((__packed__)) as suggested.

[xingxue]

Include context.

[xingxue]

libcxx/include/string

724

Changed to use anonymous struct and attribute((packed)) as suggested, thanks!

[Mordante]

Thanks for working on this!

The build failure is unrelated, can you rebase before reuploading this patch?

libcxx/include/string

726–734

Please add a comment explaining why we packed is needed.
(I understand it, but over a year I will scratch my head.)

760

Does __long have a fixed size too? If so can add a static_assert for it too?

[xingxue]

Addressed comments.

• add comments explaining why attribute __packed__ is used;
• add static_assert for struct __long.

[xingxue]

The build failure is unrelated, can you rebase before reuploading this patch?

Definitely, rebased before uploading the latest diff.

libcxx/include/string

726–734

Done, thanks!

760

static_assert for __long added, thanks!

[hubert.reinterpretcast]

libcxx/include/string

759

This assumes that pointer is not a type (maybe some sort of fancy smart pointer) that has a higher alignment requirement than sizeof(size_type) * 2.

[Mordante]

SGTM, but for now I prefer to leave the approval to people more familiar with the details of this patch and AIX.

[xingxue]

Addressed the comment to consider potential padding in the assert added for struct __long.

[xingxue]

libcxx/include/string

759

Good point, thanks! Modified the assert to take padding into account.

[hubert.reinterpretcast]

libcxx/include/string

758–759

This reduces to:

((2 * sizeof(size_type) - 1) / sizeof(pointer) + 2) * sizeof(pointer)

but I think now that there are many other ways for this to go wrong (pointer requires less alignment than its size, alignment of size_type causes trailing padding, etc.).

Maybe:

struct __long_size_ref {
  size_type __x, __y;
  pointer __z;
};
static_assert(sizeof(__long) == sizeof(__long_size_ref));

[philnik]

libcxx/include/string

758–759

How about we just drop the static_assert? I've got a patch that adds tests which static_asserts the size and alignment of specific instantiations of basic_string. That's a lot more reliable than hoping to get the size and alignment right for user-defined types.

[xingxue]

libcxx/include/string

758–759

Sounds good.

[xingxue]

Dropped the static_assert on the size of struct __long.

[hubert.reinterpretcast]

LGTM; thanks!

[philnik]

LGTM with my comment addressed.

libcxx/include/string

726–735

Could you have this comment just once above the __long and __short structs? Having it cloned makes it a lot easier for them to get out of sync.

[xingxue]

libcxx/include/string

726–735

For sure. Thanks very much for your review and good suggestions!

libcxx/include/__config

 #  if defined(_AIX) && !defined(_LIBCPP_COMPILER_GCC)
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX _Pragma("pack(1)")
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX_END _Pragma("pack(pop)")
 #  else
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX     /* empty */
 #    define _LIBCPP_PACKED_BYTE_FOR_AIX_END /* empty */
 #  endif
 
+#  if __has_attribute(__packed__)
+#    define _LIBCPP_PACKED __attribute__((__packed__))
+#  else
+#    define _LIBCPP_PACKED
+#  endif
+
 #endif // __cplusplus
 
 #endif // _LIBCPP___CONFIG

libcxx/include/string

 #else // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
 #ifdef _LIBCPP_BIG_ENDIAN
     static const size_type __endian_factor = 1;
 #else
     static const size_type __endian_factor = 2;
 #endif
 
     struct __long
     {
+        // Attribute 'packed' is used to keep the layout compatible with the
+        // previous definition that did not use bit fields. This is because
+        // on some platforms bit fields have a default size rather than
+        // the actual size used, e.g., it is 4 bytes on AIX. See D128285
+        // for details.
+        struct _LIBCPP_PACKED {
             size_type __is_long_ : 1;
             size_type __cap_ : sizeof(size_type) * CHAR_BIT - 1;
+        };
         size_type __size_;
         pointer   __data_;
     };
 
     enum {__min_cap = (sizeof(__long) - 1)/sizeof(value_type) > 2 ?
                       (sizeof(__long) - 1)/sizeof(value_type) : 2};
 
     struct __short
     {
+        // Attribute 'packed' is used to keep the layout compatible with the
+        // previous definition that did not use bit fields. This is because
+        // on some platforms bit fields have a default size rather than
+        // the actual size used, e.g., it is 4 bytes on AIX. See D128285
+        // for details.
+        struct _LIBCPP_PACKED {
             unsigned char __is_long_ : 1;
             unsigned char __size_ : 7;
+        };
         char __padding_[sizeof(value_type) - 1];
         value_type __data_[__min_cap];
     };
 
 #endif // _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 
+    static_assert(sizeof(__long) == (sizeof(pointer) + sizeof(size_type) * 2), "__long has an unexpected size.");
     static_assert(sizeof(__short) == (sizeof(value_type) * (__min_cap + 1)), "__short has an unexpected size.");
 
     union __ulx{__long __lx; __short __lxx;};
 
     enum {__n_words = sizeof(__ulx) / sizeof(size_type)};
 
     struct __raw
     {
         size_type __words[__n_words];