Avoid using std::max_align_t in pre-C++11 mode

[joerg]

Summary

Always depend on the compiler to have a correct implementation of
max_align_t in stddef.h and don't provide a fallback. For pre-C++11,
require STDCPP_NEW_ALIGNMENT in <new> as provided by clang in all
standard modes. Adjust test cases to avoid testing or using max_align_t
in pre-C++11 mode and also to better deal with alignof(max_align_t)>16.
Document requirements of the alignment tests around natural alignment of
power-of-two-sized types.

[EricWF]

the library has never had a C++03 mode, and I am against adding one now. I want libc++ to move away from C++03, not towards it

[joerg]

libc++ generally tries to play nice with C++03 code. It doesn't go out of its way to break it and keeping it usable helps dealing with a lot of rusty old code. That's what the patch is all about, not breaking things for no good reason.

[joerg]

Let me clarify the situation for a moment:
(1) libc++ does try to work in C++03 mode. See the separate implementation of <functional> for pre-C++11. It is also desirable to support. This is completely beside the question of TR1 support.
(2) The only reason why max_align_t is currently necessary is because it is used as default alignment in <new>.
(3) Most compilers we care about already have a preprocessor symbol specifically for that purpose.

If (3) is present, we shouldn't pollute the global namespace, especially with a potentially bogus value. Ideally, this shouldn't be necessary at all, since overaligned new doesn't exist in C++03. But that would be a much more intrusive change to <new> and go beyond "try not to break c++03".

[rsmith]

libc++ intentionally provides all the C++11 library functionality that it can, even when used from C++03 mode. So on the face of it, providing this name in C++03 mode seems appropriate.

However... the implementation currently used by libc++ doesn't work in that mode: the various compilers' <stddef.h>s will not provide ::max_align_t when included in C++03 mode. So either this fails to build (on NetBSD, which provides its own <stddef.h>, which I think is the issue that Joerg is reporting), or -- worse -- it silently uses the wrong definition for max_align_t by falling through to the long double fallback (which is what happens everywhere other than NetBSD). So I think we should do something here; the current approach is broken in (at least) C++03 mode.

But I think this is not the right fix. We should remove the long double fallback instead. We have no reason to think that this is correct in the cases where it's reachable (if any exist), and libc++ shouldn't be making arbitrary guesses about ABI decisions like this. Either we have the right ::max_align_t from <stddef.h> or we just can't provide one. And we should fix the defined(__NetBSD__) check to properly check for whatever conditions NetBSD defines max_align_t (maybe there's nothing better than (defined(__NetBSD__) && __cplusplus >= 201103L), but someone could look at their <stddef.h> to find out).

[joerg]

Ping?

[joerg]

Ping2?

[rsmith]

On the assumption that we will never get a ::max_align_t in C++98 mode anyway (which will be the case if the <stdlib.h> is conforming), this looks like the best we can do to me.

libcxx/include/new

229–237

Is there any ODR risk from this, or similar? Does libc++ support building in mixed C++98 / C++11 mode? If different TUs disagree on this alignment, we can end up allocating with the aligned allocator and deallocating with the unaligned allocator, which is not guaranteed to work.

We could always use the union approach if we don't know the default new alignment. But from the code below it looks like we might only ever use this if we have aligned allocation support, in which case we can just assume that the default new alignment is defined. So perhaps we can just hide the entire definition of __is_overaligned_for_new behind a #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__ and never even consider max_align_t?

[joerg]

It is used both in <new> and <memory> and the use in the latter is currently unconditional AFAICT. I don't have a problem splitting the conditional to avoid the typedef. That would address the ODR concern?

[EricWF]

libcxx/include/new

229–237

max_align_t should be ABI stable. I would rather we copy/paste the clang definition into the libc++ sources so we can use it when it's not provided by the compiler.

Though this raises another can of worms because GCC and Clang don't agree on a size for max_align_t.

232

If we're going to go this route, we should expose the __libcpp_max_align_t definition in all dialects, and compare it's size and alignment to the real max_align_t when we have it.

[joerg]

libcxx/include/new

229–237

max_align_t doesn't exist in C++03 mode, the included version is the nearest thing possible in portable C++. A visible difference happens if:
(1) The user requires C++03
(2) The code contains non-standard types with either explicit alignment or larger implicit alignment than the base types.
(3) STDCPP_DEFAULT_NEW_ALIGNMENT or alignof(max_align_t) is larger than the alignment of the union.
In that case, behavior changes as to which allocator/deallocator is used. If the explicit aligned version is not compatible and life-time crosses into c++11 mode, it could be a problem. But at that point I think we did all we could possible do to provide compatibility and the code is too far in implementation-defined land already.

[joerg]

Do not depend on max_align_t in C++03 mode in the test cases.

[EricWF]

@ldionne Since this has the possibility of breaking existing users of std::max_align_t, can you test this against your c++03 codebases?

@joerg I still don't understand why you need this change. Why can't we provide the name in C++03 mode?

I'll repeat: libc++ does not provide a C++03 mode. Only a C++11 mode that tolerates C++03 compilers.

C++03 conformance is not a valid reason for this change.

libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp

144 ↗

(On Diff #246524)

Tests under test/libcxx can use the __libcpp_max_align_t name directly, instead of being disabled.

[joerg]

libc++ has no idea what a correct max_align_t is. The internal definition works due to historic requirements that all three fundamental types are supported for new/delete, but we don't have any such guarantees for every other context. A correctly implemented stddef.h does not provide max_align_t in C++03 mode, since that would pollute the global namespace. This means that libc++ currently has two failure modes: on NetBSD, it outright tries to use a non-existing symbol. On other platforms it silently defines max_align_t in a way that can be subtle wrong.

I should add that e.g. libstdc++ doesn't provide it either, so at least somewhat portable C++03 code can not depend on the presence anyway.

[ldionne]

In D73245#1894420, @EricWF wrote:

@ldionne Since this has the possibility of breaking existing users of std::max_align_t, can you test this against your c++03 codebases?

I have no opinion about this patch, but I generally agree we shouldn't make efforts to provide C++03 conformance. It's a vain effort and we should be looking towards the future. On the other hand, this patch does seem to simplify the code a bit, in the sense that we now don't provide anything unless we have a conforming C++11 implementation, in which case we don't need to go through platform specific hoops anymore. I like that.

Regardless, I'm running this change on a code base to see whether it causes problems.

[ldionne]

In D73245#1896786, @ldionne wrote:

In D73245#1894420, @EricWF wrote:

@ldionne Since this has the possibility of breaking existing users of std::max_align_t, can you test this against your c++03 codebases?

LGTM as far as that is concerned. (personal note: rdar://60008079)

[joerg]

Ping

[EricWF]

I've already stated my disapproval of this patch. Libc++ has never and will never provide nor value C++03 conformance.
Moving backwards to C++03 is inconsistent with the libraries general direction.

This patch disables tests, which could hide bugs, including serious ABI differences between dialects.

I would like to unbreak compilation on NetBSD. But all that's needed there is to provide our own correct declaration of max_align_t.
I don't see why C++03 conformance is a necessary condition.

If you would still like to proceed in this direction. Please remove me as a reviewer.

libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp

156 ↗

(On Diff #246524)

It's important these tests continue to pass in C++03.

[ldionne]

In D73245#1918148, @EricWF wrote:

I've already stated my disapproval of this patch. Libc++ has never and will never provide nor value C++03 conformance.
Moving backwards to C++03 is inconsistent with the libraries general direction.

@EricWF makes a point here, we want to move away from C++03.

This patch disables tests, which could hide bugs, including serious ABI differences between dialects.

I would like to unbreak compilation on NetBSD. But all that's needed there is to provide our own correct declaration of max_align_t.
I don't see why C++03 conformance is a necessary condition.

Is there anything that can be done on the NetBSD side to solve this? Basically, just imagine that libc++ doesn't provide a C++03 mode at all -- what would you do then? I think that's the right mindset to solve this specific problem.

[joerg]

In D73245#1918287, @ldionne wrote:

In D73245#1918148, @EricWF wrote:

I've already stated my disapproval of this patch. Libc++ has never and will never provide nor value C++03 conformance.
Moving backwards to C++03 is inconsistent with the libraries general direction.

@EricWF makes a point here, we want to move away from C++03.

Removing or breaking C++03 compatibility is actively harmful for that. Speaking as a system integrator, a STL implementation that can't deal in a reasonable way with the existing C++03 code base is useless. It's a nice ivory tower assumption that old code will just disappear if you wish for it enough, but it doesn't reflect well on the reality. Especially given the various ways can and does fail when compiled for C++11 or later. Having to switch between different STL implementations doesn't help anyone in the process of modernizing a code base, especially when interacting with code that needs large semi-intrusive changes to work in C++11 mode. As a most basic issue, it makes it much harder to separate issues with the STL implementation (i.e. depending on implementation-defined behavior) from semantic changes in the core language (i.e. noexcept dtors, auto) and mechanical changes (e.g. UDL)

This patch disables tests, which could hide bugs, including serious ABI differences between dialects.

I would like to unbreak compilation on NetBSD. But all that's needed there is to provide our own correct declaration of max_align_t.
I don't see why C++03 conformance is a necessary condition.

Is there anything that can be done on the NetBSD side to solve this? Basically, just imagine that libc++ doesn't provide a C++03 mode at all -- what would you do then? I think that's the right mindset to solve this specific problem.

Can you both please go back to look at the core of the issue? The current behavior of libc++ is at best an ODR violation on ALL platforms. This is not a problem specific to NetBSD at all, it has just been exposed as a different error on NetBSD. The documented behavior of libc++ is to provide C++03 compatibility as much as possible when it doesn't conflict with C++11 and the compiler (clang) supports the necessary language feature. max_align_t is not provided by clang in C++03 nor is it provided by GCC. Now there are different options for fixes, but it is very frustrating to me that the primary feedback so far is "Don't use C++03" and therefore not helpful at all. As I said before, libc++ cares about max_align_t exactly for one purpose: whether overaligned types need special allocators. Now C++03 normally can't depend on the implementation to do something useful in this case, simply because the environment doesn't have the necessary means. So it all boils down to what is the least intrusive and potentially most well testable mechanism. The core of the patch provides one answer for that. If the compiler supports telling us the default alignment for new, use it just like we would in C++11 mode. A constructive feedback would now be an answer to the question of whether we care about libc++ use with compilers that do not provide that macro for C++03 mode. I don't know how useful libc++ in C++03 mode is for any compiler except clang and as I wrote, we document the primary target being clang. As such the primary question is whether we care about compatibility with older versions of clang and modern libc++. If we don't, the change to <new> becomes much simpler as it can just bail out and the corresponding testing becomes easier as well, since it would only have to assert that max_align_t is aligned less strictly than the platform allocator. It would solve the core issue that we provide a max_align_t when we don't know what it should be and actively disagree with the max_align_t we provide for the other dialects.

[ldionne]

libcxx/include/new

240

So, IIUC what you're saying, __STDCPP_DEFAULT_NEW_ALIGNMENT__ is provided by recent Clangs in C++03 mode? I tested it and it does seem to be correct. (side note: I tend to think we should be more aggressive to remove old compiler support, since most people don't upgrade their stdlib without upgrading their compiler anyway).

So if we don't care about supporting old compilers that don't provide that macro, we could just get rid of this #elif, and such compilers would error out when trying to use max_align_t in the #else below. That appears reasonable to me.

We'd still leave the #if TEST_STD_VER >= 11 in the tests below, since in C++03 we wouldn't provide std::max_align_t, however testing that we use overaligned new in the same conditions in C++03 and C++11 becomes trivial, because it's the same code path.

Did I get what you meant correctly? If so, that sounds like a viable path forward to me, since we're simplifying the code. We're also improving on our C++03 conformance, which isn't considered good but is certainly not considered bad either.

[joerg]

libcxx/include/new

240

Correct, it has been provided since clang 4.0 at least it seems. For testing, we have two cases, some that specifically check properties of max_align_t and those should be restricted to C++11 and newer. I think we should grow a new check that max_align_t <= STDCPP_DEFAULT_NEW_ALIGNMENT as sanity check, but that's slightly OT. Most of the existing cases to check for overalignment already use STDCPP_DEFAULT_NEW_ALIGNMENT anyway, so it would be a case-by-case check for those.

[ldionne]

libcxx/include/new

240

I'm fine with that direction if you're willing to update the patch. I'll review it.

[krytarowski]

libcxx/include/new

240

Do I understand it correctly that instead of __libcpp_max_align_t (or (over)exposed max_align_t) we can rely entirely on __STDCPP_DEFAULT_NEW_ALIGNMENT__?

[ldionne]

libcxx/include/new

240

Yes, that is the plan IIUC.

[joerg]

Require __STDCPP_NEW_ALIGNMENT__ in C++03 mode. Prefer it over max_align_t in a number of tests when allocation alignment is desired. Adjust some tests to do minimal sanity checking of the alignment for C++03 only. Add an explicit check that __STDCPP_NEW_ALIGNMENT__ is more general than max_align_t if both are present.

[ldionne]

I think the direction works, but I have questions about some thing I don't understand (mostly in the tests).

libcxx/include/stddef.h

53–54

Why do we need this at all anymore? In C++11, can't we rely on the C Standard Library providing it ::max_align_t?

libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp

40 ↗

(On Diff #250937)

This test is only enabled in >= C++11, so I think std::max_align_t should always be provided. So why do we want that #if?

libcxx/test/std/language.support/support.types/max_align_t.pass.cpp

47 ↗

(On Diff #250937)

Since we now assume that __STDCPP_DEFAULT_NEW_ALIGNMENT__ is defined, can we remove that #if and keep the assertion?

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

275 ↗

(On Diff #250937)

Can you walk me through why the check for > 16 is required?

[joerg]

libcxx/include/stddef.h

53–54

Sorry, left-over. Will be killed.

libcxx/test/libcxx/experimental/memory/memory.resource.adaptor/memory.resource.adaptor.mem/db_deallocate.pass.cpp

40 ↗

(On Diff #250937)

We know that the new alignment can be stricter than max_align_t and we want to test the overflow case here. So going for the strictest alignment is more sensible.

libcxx/test/std/language.support/support.types/max_align_t.pass.cpp

47 ↗

(On Diff #250937)

It's only required in C++03 mode. We still support C++11 and higher without it, e.g. for gcc.

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

275 ↗

(On Diff #250937)

If max_align_t is 256bit, we still only expect 128bit alignment (long double). This test is still checking more than it should, e.g. in principle a target could legitimately have no natural type larger than 64bit, but support 256bit vector types and set max_align_t accordingly. The condition is here because std::min in C++11 is not constexpr.

[ldionne]

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

275 ↗

(On Diff #250937)

Naively, I would have expected the test to be:

#if TEST_STD_VER >= 11 // we know max_align_t is provided
  static_assert(std::alignment_of<T1>::value == TEST_ALIGNOF(std::max_align_t), "");
#else
  static_assert(std::alignment_of<T1>::value >= TEST_ALIGNOF(natural_alignment), "");
#endif

To make sure I understand, you're saying we can't do that because the std::alignment_of<T1>::value == TEST_ALIGNOF(std::max_align_t) test would sometimes fail if alignof(std::max_align_t) is larger than the natural alignment used by std::aligned_storage. This also highlights that there's a change in the value of alignof(std::max_align_t) with this change. Is that correct?

[joerg]

libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp

275 ↗

(On Diff #250937)

Yes, the test would have failed before when max_align_t > 16. There is no change of value in alignof(max_align_t), just seen by review.

[joerg]

Ping?

[joerg]

ping2

Louis, did I answer your questions?

[ldionne]

LGTM

libcxx/include/cstddef

 
 namespace std
 {
 
 Types:
 
     ptrdiff_t
     size_t
-    max_align_t
+    max_align_t // C++11
     nullptr_t
     byte // C++17
 
 }  // std
 
 */
 
 #include <__config>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 // Don't include our own <stddef.h>; we don't want to declare ::nullptr_t.
 #include_next <stddef.h>
 #include <__nullptr>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 using ::ptrdiff_t;
 using ::size_t;
 
-#if defined(__CLANG_MAX_ALIGN_T_DEFINED) || defined(_GCC_MAX_ALIGN_T) || \
-    defined(__DEFINED_max_align_t) || defined(__NetBSD__)
-// Re-use the compiler's <stddef.h> max_align_t where possible.
+#if !defined(_LIBCPP_CXX03_LANG)
 using ::max_align_t;
-#else
-typedef long double max_align_t;
 #endif
 
 template <class _Tp> struct __libcpp_is_integral                     { enum { value = 0 }; };
 template <>          struct __libcpp_is_integral<bool>               { enum { value = 1 }; };
 template <>          struct __libcpp_is_integral<char>               { enum { value = 1 }; };
 template <>          struct __libcpp_is_integral<signed char>        { enum { value = 1 }; };
 template <>          struct __libcpp_is_integral<unsigned char>      { enum { value = 1 }; };
 template <>          struct __libcpp_is_integral<wchar_t>            { enum { value = 1 }; };

libcxx/include/new

 _LIBCPP_NODISCARD_AFTER_CXX17 inline _LIBCPP_INLINE_VISIBILITY void* operator new[](std::size_t, void* __p) _NOEXCEPT {return __p;}
 inline _LIBCPP_INLINE_VISIBILITY void  operator delete  (void*, void*) _NOEXCEPT {}
 inline _LIBCPP_INLINE_VISIBILITY void  operator delete[](void*, void*) _NOEXCEPT {}
 
 #endif // !_LIBCPP_ABI_VCRUNTIME
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
+#if defined(_LIBCPP_CXX03_LANG)
+union __libcpp_max_align_t {
+  void * __f1;
+  long long int __f2;
+  long double __f3;
+};
+#endif
+
 _LIBCPP_CONSTEXPR inline _LIBCPP_INLINE_VISIBILITY bool __is_overaligned_for_new(size_t __align) _NOEXCEPT {
 #ifdef __STDCPP_DEFAULT_NEW_ALIGNMENT__
   return __align > __STDCPP_DEFAULT_NEW_ALIGNMENT__;
+#elif defined(_LIBCPP_CXX03_LANG)
+  return __align > alignment_of<__libcpp_max_align_t>::value;
 #else
   return __align > alignment_of<max_align_t>::value;
 #endif
 }
 
 inline _LIBCPP_INLINE_VISIBILITY void *__libcpp_allocate(size_t __size, size_t __align) {
 #ifndef _LIBCPP_HAS_NO_ALIGNED_ALLOCATION
   if (__is_overaligned_for_new(__align)) {

libcxx/include/stddef.h

 #include_next <stddef.h>
 
 #ifdef __cplusplus
 
 extern "C++" {
 #include <__nullptr>
 using std::nullptr_t;
 }
 
-// Re-use the compiler's <stddef.h> max_align_t where possible.
-#if !defined(__CLANG_MAX_ALIGN_T_DEFINED) && !defined(_GCC_MAX_ALIGN_T) && \
-    !defined(__DEFINED_max_align_t) && !defined(__NetBSD__)
-typedef long double max_align_t;
-#endif
-
 #endif
 
 #endif  // _LIBCPP_STDDEF_H