This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.

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.Jan 30 2020, 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.Jan 30 2020, 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.Feb 4 2020, 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 ↗(On Diff #242317)

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.Feb 19 2020, 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.Feb 19 2020, 4:22 PM
libcxx/include/new
229–237 ↗(On Diff #242317)

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 ↗(On Diff #242317)

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.Feb 20 2020, 4:58 AM
joerg added inline comments.
libcxx/include/new
229–237 ↗(On Diff #242317)

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.Feb 21 2020, 7:23 AM
joerg updated this revision to Diff 246524.Feb 25 2020, 11:48 AM

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

EricWF added a subscriber: ldionne.Feb 26 2020, 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
145

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

joerg added a comment.Feb 26 2020, 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.

@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 added a comment.EditedMar 5 2020, 12:54 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?

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

EricWF requested changes to this revision.Mar 11 2020, 3:03 PM

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
159

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

This revision now requires changes to proceed.Mar 11 2020, 3:03 PM

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.

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 added inline comments.Mar 12 2020, 4:07 PM
libcxx/include/new
240 ↗(On Diff #246524)

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 added inline comments.Mar 13 2020, 2:13 AM
libcxx/include/new
240 ↗(On Diff #246524)

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 requested changes to this revision.Mar 13 2020, 7:56 AM
ldionne added inline comments.
libcxx/include/new
240 ↗(On Diff #246524)

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

krytarowski added inline comments.Mar 17 2020, 4:56 AM
libcxx/include/new
240 ↗(On Diff #246524)

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 added inline comments.Mar 17 2020, 12:58 PM
libcxx/include/new
240 ↗(On Diff #246524)

Yes, that is the plan IIUC.

joerg updated this revision to Diff 250937.Mar 17 2020, 4:10 PM
joerg edited the summary of this revision. (Show Details)

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 requested changes to this revision.Mar 20 2020, 8:19 AM

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

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

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

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

This revision now requires changes to proceed.Mar 20 2020, 8:19 AM
joerg marked 5 inline comments as done.Mar 20 2020, 1:04 PM
joerg added inline comments.
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

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

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

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 added inline comments.Mar 21 2020, 7:25 AM
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
275

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 marked 2 inline comments as done.Mar 21 2020, 2:46 PM
joerg added inline comments.
libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/aligned_storage.pass.cpp
275

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 retitled this revision from Depend stddef.h to provide max_align_t for C++11 and provide better fallback in <new> to Avoid using std::max_align_t in pre-C++11 mode.Mar 28 2020, 3:16 PM
joerg edited the summary of this revision. (Show Details)
joerg added a comment.Apr 3 2020, 8:10 AM

ping2

Louis, did I answer your questions?

ldionne accepted this revision.Apr 3 2020, 11:35 AM

LGTM

This revision was not accepted when it landed; it landed in state Needs Revision.Apr 3 2020, 4:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 4:49 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript