[libc++] Initialization of std::array of size 0 in constexpr contexts

[mjacobse]

constexpr std::array<int, 0> a;

This fails to compile with libc++ (https://godbolt.org/z/drcGsW84x), since its zero-length std::array specialization has some dummy data (see #34839) that is left uninitialized.

This is quite unfortunate for a case like

template <int N>
constexpr auto getOnes() {
    std::array<int, N> a;
    a.fill(1);
    return a;
}

which will fail to compile only when used in a constexpr context with N = 0 (https://godbolt.org/z/jxMGx1941). Using std::array<int, N> a{}; fixes that, but it may also hide programming errors (i.e. missed elements) in the following actually intended initialization code for N > 0 that would otherwise be diagnosed.

Microsoft's STL was in a very similar situation and fixed this just recently (see microsoft/STL#3863), so I was hoping that maybe libc++ might want to follow. Seems like for libstdc++ this was never a problem since their std::array<T, 0> is actually empty.

[philnik777]

The standard is unfortunately notoriously underspecified in these regards. I don't really see a neat way to do this without incurring potentially significant runtime overhead. The best thing would probably have been to never add any data in an empty array, but that opportunity is long gone.

[rnicholl-google]

I think this requires an ABI flag to fix, but that is probably the correct solution?

[philnik777]

Well yeah, in the unstable ABI it's trivial to fix. I was thinking about the stable ABI, which probably 90%+ of people use.

[rnicholl-google]

I think I will add this macro in the ABI version 2:

// Make std::array<T, N> where N == 0 have no data
// This allows it to be used in constexpr contexts without
// introducing runtime overhead
#    define _LIBCPP_ABI_ZERO_SIZE_ARRAY_HAS_NO_DATA

Any objections?

[philnik777]

I don't have an objection to doing this, but I do object to this being a fix for the bug.

[rnicholl-google]

If we want to fix this in the stable ABI as well, does this require us to initialize the member in all cases?

I'm not sure if this can be fixed without compiler support. I could add code to initialize it with = {} starting from C++14 onwards, but in C++11 the object would no longer be considered an aggregate... not sure how to fix that without adding some kind of compiler builtin for this special case? Also that solution adds runtime overhead, which I think is bad.

[philnik777]

These are all great questions which I don't have the answers to. Worst case we'd have to add some minor compiler magic, but IMO that's better than not fixing this for most people. Maybe the runtime overhead isn't really that bad. I mean, how often does it actually happen that you initialize a bunch of empty arrays? Probably almost never, but I don't know.

[rnicholl-google]

This extra data field might have some additional consequences that removing it does also break the current API.

Looks like expressions like:

std::array<T, 0> a{{}}; 
std::array<T, 0> a{0}; 

Currently compile. I am fairly confident they should not compile if the array has a size of 0, but what is the policy around breaking API compatibility here?

[rnicholl-google]

Looks like there is an open issue related to this:

https://cplusplus.github.io/LWG/issue2157

I am not sure I like the idea of {{}} being required to compile, but maybe we should track that? I don't think this proposal has been adopted into the standard yet.

[mjacobse]

Looks like there is an open issue related to this:

https://cplusplus.github.io/LWG/issue2157

This is actually what is addressed in #34839 which is why I referenced it, although there seem to have been quite a few updates since then. Unfortunately it looks like the corresponding commit 59cdf90 which explicitly mentions LWG 2157 was not linked with that issue in the migration to GitHub? See also microsoft/STL#2296 for their discussion.

[rnicholl-google]

it seems like adding a zero length array gets us semantics similar to glibc. The {{}} syntax evidently needs to continue to work, but I think not allowing {0} to compile anymore is okay. That doesn't work elsewhere aside from libc++ anyway.

Edit: actually no, this is different in some corner cases.

[ldionne]

This patch is also relevant: 7265ff9

commit 7265ff928a974a844b6301c139cbb0f957532da9
Author: Louis Dionne <ldionne@apple.com>
Date:   Fri May 29 16:32:55 2020 -0700

    [libc++] Fix issues with the triviality of std::array

    The Standard is currently unimplementable. We have to pick between:

    1. Not implementing constexpr support properly in std::array<T, 0>
    2. Making std::array<T, 0> non-trivial even when T is trivial
    3. Returning nullptr from std::array<T, 0>::begin()

    Libc++ initially picked (1). In 77b9abfc8e89, we started implementing 
    constexpr properly, but lost the guarantee of triviality. Since it seems 
    like both (1) and (2) are really important, it seems like (3) is the only 
    viable option for libc++, after all. This is also what other implementations 
    are doing.

    This patch moves libc++ from (1) to (3).

    It also:
    - Improves the test coverage for the various ways of initializing std::array
    - Adds tests for the triviality of std::array
    - Adds tests for the aggregate-ness of std::array

    Differential Revision: https://reviews.llvm.org/D80821

However, while this patch moved std::array<T, 0> from (1) to (3), we still didn't get support for a default-initialized array in constexpr contexts, as evidenced by our array.cons/initialization.pass.cpp test.

[ldionne]

Actually, it looks like the following code works just fine in C++20 mode:

constexpr std::array<int, 0> a;

It only fails in C++11, C++14 and C++17. https://godbolt.org/z/3nq8MTax5

It also seems like libstdc++ and libc++ agree exactly on when it should work vs not work, and that what we implement is the resolution of LWG2157 (which wasn't officially voted into the standard, though).

I'm tempted to say that this behaves correctly. The only thing we could potentially do is reduce the size of std::array<T, 0> to 1 byte under an ABI flag, but I think the benefit is likely pretty small.

[ldionne]

I am going to tentatively close this as NTBF to keep the issues list tidy and make sure this doesn't stay open forever, but if you think this should stay open, let's discuss (and reopen).

[rnicholl-google]

I am curious why std::array<T, 0>::begin() needs to return nullptr, is it because we cannot reinterpret cast in a constexpr function?

I have a fix for the original complaint.

An array of _EmptyAggrgate can be used to maintain ABI compatibility while allowing default construction.

[ldionne]

I am curious why std::array<T, 0>::begin() needs to return nullptr, is it because we cannot reinterpret cast in a constexpr function?

Yes, exactly.

I have a fix for the original complaint.

An array of _EmptyAggrgate can be used to maintain ABI compatibility while allowing default construction.

Can you expand on the fix you suggest? I'm not sure I see what you mean.

Regardless I think it's worth keeping this open since I had misunderstood the original report. It looks like while

constexpr void f() {
  std::array<T, 0> a;
}

works, the original report was about constexpr std::array<T, 0> a; which indeed doesn't work. I don't quite understand how they're different, but they behave differently.

[rnicholl-google]

I am curious why std::array<T, 0>::begin() needs to return nullptr, is it because we cannot reinterpret cast in a constexpr function?

Yes, exactly.

I am not convinced this is a problem. array<T,0>::begin() returns an iterator, as far as I'm aware, nothing says that std::array<T,N>::iterator has to be a T*. We might as well create a special iterator for the array<T, 0>::iterator case to maintain the uniqueness requirement.

[rnicholl-google]

Drafted a pull request, #74657

This doesn't have an iterator fix though, which I'd like to fix as well while I'm at it.

[rnicholl-google]

I'm adding "_EmptyArrayIterator" which to the best of my knowledge, satisfies the C++ standard requirements.

For some compatibility with code of the form:

T * it = arr.begin();
T * it_end = arr.end();

I am making _EmptyArrayIterator implictly convertible to T*, which returns a nullptr when doing the conversion.

It is possible this could trigger an API break, since the return type of auto begin = arr.begin() will no longer be T*. However, I don't see any way to avoid this while conforming to the C++ standard. To avoid too many surprises with compiling code that 'used to work', I'll only implement the standard conformant iterator in the unstable ABI version.

[ldionne]

It looks like we raced on this issue. I also have a different PR -- I'll open it and we can take a look at both and decide what to do. I have to go now but I'll take a look in the morning.

[rnicholl-google]

It looks like we raced on this issue. I also have a different PR -- I'll open it and we can take a look at both and decide what to do. I have to go now but I'll take a look in the morning.

I have used a similar fix, but I also added an ABI change (to make the size 1 byte on the unstable abi). However, I'm not convinced that std::array<T const, 0> val should be non-copy assignable. I'm not sure how to enable empty base optimization and also make array<T const,0> un-assignable.

[rnicholl-google]

I sent some messages to std-proposals, but it doesn't look like the standard specifies whether or not they are allowed. I suggest we follow glibc and allow them though.

[rnicholl-google]

I'm adding "_EmptyArrayIterator" which to the best of my knowledge, satisfies the C++ standard requirements.

For some compatibility with code of the form:

T * it = arr.begin();
T * it_end = arr.end();

I am making _EmptyArrayIterator implictly convertible to T*, which returns a nullptr when doing the conversion.

It is possible this could trigger an API break, since the return type of auto begin = arr.begin() will no longer be T*. However, I don't see any way to avoid this while conforming to the C++ standard. To avoid too many surprises with compiling code that 'used to work', I'll only implement the standard conformant iterator in the unstable ABI version.

It turns out this doesn't work either. I think I agree... the standard behavior is not possible to implement. Somewhere along the line (I think C++20? didn't check) they added a requirement that array<T, N>::iterator is a contiguous iterator, which obviously isn't possible using a custom iterator class.

It could be done with a compiler builtin, but there's no way to do it with generic syntax.

[EricWF]

Going back to begin() and end() returning nullptr. I feel the standard explicitly prohibits that when it says:

In the case that N == 0, begin() == end() == unique value. The return value of data() is unspecified.

The unique value mandates that two distinct array objects return different values for begin, even when empty.

Why did we change that? I need to better understand that to consider what to best do here.

[EricWF]

Sorry, I should have read further. The context regarding begin() and nullptr is there.

However, what I don't get is this. The standard is VERY EXPLICIT about the begin() != nullptr requirement, and significantly less clear on everything else.

It seems to me we would implement the behavior most clearly specified, and then ask the standard to figure its shit out.

It seems like implementations here are papering over an unimplementable specification to the pain of all of our users.
This needs a fix, but it needs to be in the standard.

[rnicholl-google]

I think we need to add a compiler builtin to get that to work as written. However, I think it's better for someone to pursue this as a defect report in the standard.

[rnicholl-google]

Correction, I think I might have found a solution to maintain standard conformance. It requires a bit of hackery since the C++11 and C++14+ solutions aren't compatible. (I realized, I can just reinterpret cast in C++11 since begin isn't required to be constexpr there) I'll update my patch with this experimental solution idea and see if I can get it to pass the existing tests maybe.

[rnicholl-google]

Okay, my fix should theoretically work, but it doesn't work because it's blocked by this bug: #56814