Page MenuHomePhabricator

Depend stddef.h to provide max_align_t for C++11 and provide better fallback in <new>
Needs ReviewPublic

Authored by joerg on Jan 22 2020, 5:15 PM.

Details

Summary

Depend on the compiler to provide a correct implementation of
max_align_t. If __STDCPP_NEW_ALIGNMENT__ is missing and C++03 mode has
been explicitly enabled, provide a minimal fallback in <new> as
alignment of the largest primitive types.

Diff Detail

Event Timeline

joerg created this revision.Jan 22 2020, 5:15 PM
EricWF requested changes to this revision.Jan 22 2020, 5:54 PM
EricWF added a subscriber: 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

This revision now requires changes to proceed.Jan 22 2020, 5:54 PM
joerg added a comment.Jan 22 2020, 6:11 PM

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 added a comment.Thu, Jan 30, 4:51 PM

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 added a subscriber: rsmith.Thu, Jan 30, 5:41 PM

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 updated this revision to Diff 242317.Tue, Feb 4, 6:24 AM
joerg retitled this revision from Don't define std::max_align_t if not used in C++03 mode to Depend stddef.h to provide max_align_t for C++11 and provide better fallback in <new>.
joerg edited the summary of this revision. (Show Details)

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 added a comment.Wed, Feb 19, 2:42 PM

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 added inline comments.Wed, Feb 19, 4:22 PM
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 marked an inline comment as done.Thu, Feb 20, 4:58 AM
joerg added inline comments.
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 updated this revision to Diff 245853.Fri, Feb 21, 7:23 AM
joerg updated this revision to Diff 246524.Tue, Feb 25, 11:48 AM

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

EricWF added a subscriber: ldionne.Wed, Feb 26, 2:05 PM

@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

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

joerg added a comment.Wed, Feb 26, 2:57 PM

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.