[cfe-dev] libc++ patch for max_align_t

[Kal]

Am 10.02.14 19:33, schrieb Howard Hinnant:
> Because of 17.6.5.2 [res.on.headers]/p3, I do not believe we should make <cstddef> dependent upon (include) <cstdint> or <stdint.h>.  Therefore <cstddef> should not reference the typedef uintmax_t.  That being said, we could certainly include the aliased type for uintmax_t on any platform we wish to target.  On Apple platforms this would be unsigned long long, and for archaic reasons, also unsigned long.
>
> While investigating your question, I think I've uncovered an ancient bug in libc++.  The defaulted alignment argument for aligned storage is supposed to be:
>
>> The value of default-alignment shall be the most stringent alignment requirement for any C++ object type whose size is no greater than Len (3.9).
> If you output the value of __find_max_align<__all_types, _Len>::value for many values of _Len on OS X, you get:
>
> std::__default_align<1>::value = 1
> std::__default_align<2>::value = 2
> std::__default_align<3>::value = 2
> std::__default_align<4>::value = 4
> std::__default_align<5>::value = 4
> std::__default_align<6>::value = 4
> std::__default_align<7>::value = 4
> std::__default_align<8>::value = 8
> std::__default_align<9>::value = 8
> std::__default_align<10>::value = 8
> std::__default_align<11>::value = 8
> std::__default_align<12>::value = 8
> std::__default_align<13>::value = 8
> std::__default_align<14>::value = 8
> std::__default_align<15>::value = 8
> std::__default_align<16>::value = 16
> std::__default_align<17>::value = 16
> std::__default_align<18>::value = 16
> std::__default_align<19>::value = 16
> std::__default_align<20>::value = 16
> std::__default_align<21>::value = 16
> std::__default_align<22>::value = 16
> std::__default_align<23>::value = 16
> std::__default_align<24>::value = 16
> std::__default_align<25>::value = 16
> std::__default_align<26>::value = 16
> std::__default_align<27>::value = 16
> std::__default_align<28>::value = 16
> std::__default_align<29>::value = 16
> std::__default_align<30>::value = 16
> std::__default_align<31>::value = 16
> std::__default_align<32>::value = 16
> std::__default_align<33>::value = 16
> ...
>
> I.e. The default alignement for an object 17 bytes big is 16.
>
> However on reflection, I believe this is incorrect. I believe it should look like:

It seems like the old behavior follows the standard more closely. If Len
== 17, then _any_ c++ object with size no greater than Len includes
objects of size 16 and some objects of size 16 maybe have an alignment
requirement of 16.

>
> std::__default_align<1>::value = 1
> std::__default_align<2>::value = 2
> std::__default_align<3>::value = 1
> std::__default_align<4>::value = 4
> std::__default_align<5>::value = 1
> std::__default_align<6>::value = 2
> std::__default_align<7>::value = 1
> std::__default_align<8>::value = 8
> std::__default_align<9>::value = 1
> std::__default_align<10>::value = 2
> std::__default_align<11>::value = 1
> std::__default_align<12>::value = 4
> std::__default_align<13>::value = 1
> std::__default_align<14>::value = 2
> std::__default_align<15>::value = 1
> std::__default_align<16>::value = 16
> std::__default_align<17>::value = 1
> std::__default_align<18>::value = 2
> std::__default_align<19>::value = 1
> std::__default_align<20>::value = 4
> std::__default_align<21>::value = 1
> std::__default_align<22>::value = 2
> std::__default_align<23>::value = 1
> std::__default_align<24>::value = 8
> std::__default_align<25>::value = 1
> std::__default_align<26>::value = 2
> std::__default_align<27>::value = 1
> std::__default_align<28>::value = 4
> std::__default_align<29>::value = 1
> std::__default_align<30>::value = 2
> std::__default_align<31>::value = 1
> std::__default_align<32>::value = 16
> std::__default_align<33>::value = 1
> ...
>
> In terms of a formula, I believe the default alignment should be computed by:
>
>   default_alignment(Len) = min(1 << ctz(Len), alignof(max_align_t));
>
> Do others agree there is a bug here, and that this is the correct fix?  If so, I will prepare another patch to address this.  And I think this new patch will somewhat simplify the current solution for computing max_align_t in the absence of __ALIGNOF_MAX_ALIGN_T__.  Simplification is good, it will lead to slightly faster compile times.
>
> Thanks,
> Howard
>
> On Feb 10, 2014, at 2:38 AM, Kal <b17c0de at gmail.com> wrote:
>
>> Hi Howard,
>> Your solution looks neat. Wouldn't it make sense to also include
>> 'uintmax_t' in the type_list? Also for the aligned_storage use case,
>> seems like you should also be including 'float'?
>> -Kal
>>
>> Am 09.02.14 20:50, schrieb Howard Hinnant:
>>> On Feb 9, 2014, at 12:03 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>>>
>>>> On the CFE-dev list, Kal reported:
>>>>
>>>>> Running libc++ 3.4 rc1 "testit" on 32-bit Linux fails for test:
>>>>>
>>>>> test/language.support/support.types/max_align_t.pass.cpp
>>>>>
>>>>> max_align_t is typedef'd to "long double" type in <cstddef>. But...
>>>>>
>>>>> alignment_of(long double)=4, sizeof(long double)=12
>>>>> alignment_of(long long)=8, sizeof(long long)=8
>>>> and last night, in r201037, David Majnemer added support for a macro __ALIGNOF_MAX_ALIGN_T__.
>>>>
>>>> This patch changes the definition of max_align_t when that macro is present.
>>>>
>>>> Note: The actual definition of max_align_t is:
>>>> • The type max_align_t is a POD type whose alignment requirement is at least as great as that of every scalar type, and whose alignment requirement is supported in every context.
>>>>
>>>> — Marshall
>>>>
>>>> <max_align.patch>
>>> Thanks Kal and Marshall!
>>>
>>> I think this is a good patch.  However I have a counter-proposal that is a bit more ambitious:
>>>
>>> This patch moves the private __find_max_align utility from <type_traits> to <cstddef> (<type_traits> includes <cstddef>).  This utility creates a list of likely types, and their alignments, and then given a sizeof, it selects an appropriate alignment.  This is used in the implementation of std::aligned_storage_t for the defaulted alignment parameter.
>>>
>>> The purpose of moving this utility is to fall back on it in case we can't find max_align_t in <stddef.h>, and in case __ALIGNOF_MAX_ALIGN_T__ is not already defined by the compiler (or I supposed by <__config> for some platform).  This is simply done by getting the alignment for a ridiculously large type, say sizeof == 1Kb:
>>>
>>> #ifndef __ALIGNOF_MAX_ALIGN_T__
>>> #define __ALIGNOF_MAX_ALIGN_T__ (__find_max_align<__all_types, 1024>::value)
>>> #endif
>>>
>>> On OSX / iOS, __find_max_align<__all_types, 1024>::value == 16.  If some platform needs to add a type to the type list __all_types, that is easily done.  But this facility has been used in aligned_storage for years, and no one has yet had any problems with it.
>>>
>>> Features:
>>>
>>> 1. If <__config> defines _LIBCPP_C_HAS_MAX_ALIGN_T, then max_align_t is grabbed from the global namespace / <stddef.h>.  Currently no platform in <__config> defines _LIBCPP_C_HAS_MAX_ALIGN_T.  C11 introduces max_align_t in <stddef.h>.  I expect that eventually all platforms will migrate to this option.
>>>
>>> 2. If _LIBCPP_C_HAS_MAX_ALIGN_T is not defined, but __ALIGNOF_MAX_ALIGN_T__ is defined, then max_align_t is created as proposed by Marshall.  As Marshall noted, Kal has just added __ALIGNOF_MAX_ALIGN_T__ to the tip-of-trunk clang.
>>>
>>> 3. If neither _LIBCPP_C_HAS_MAX_ALIGN_T nor __ALIGNOF_MAX_ALIGN_T__ is defined, then __ALIGNOF_MAX_ALIGN_T__ is defined as (__find_max_align<__all_types, 1024>::value), and then again max_align_t is created as proposed by Marshall.  This latter step is I believe a better default than what we originally had:  long double.
>>>
>>> All of the code dealing with alignment is pretty ancient in libc++, and it was overdue for an update.  Along the way I've modernized and simplified aligned_storage.  And there's a drive-by fix of a C++03 bug in common_type<int> pointed out by Marshall.  And another drive-by fix of a C++03 warning in test/utilities/meta/meta.unary/meta.unary.prop/is_assignable.pass.cpp.
>>>
>>> I've tested with Apple's clang-500.0.68 (which lacks __ALIGNOF_MAX_ALIGN_T__), in C++03 and C++11, and with tip-of-trunk clang (which has __ALIGNOF_MAX_ALIGN_T__) in C++03, C++11 and C++1y.  The branch which imports max_align_t from <stddef.h> has not been tested.  But this branch consists of only a single line of code:
>>>
>>> using ::max_align_t;
>>>
>>> Howard