Page MenuHomePhabricator

[libcxx] Return "real" pointer from array<T, 0>::data.
Needs RevisionPublic

Authored by zoecarver on Jun 4 2020, 9:52 AM.

Details

Reviewers
ldionne
EricWF
miscco
Group Reviewers
Restricted Project
Summary

Brings back the "union trick" so that we can stop returning nullptr from data and range methods. This patch adds two overloads of the union __array_storage_wrapper. One for trivially copyable types, and one for non-trivially copyable types (where we define a copy constructor).

Diff Detail

Event Timeline

zoecarver created this revision.Jun 4 2020, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 9:52 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
miscco added a comment.Jun 4 2020, 9:57 AM

I am still having trouble to unterstand what benefit this has.

I am still having trouble to unterstand what benefit this has.

The benefit is two-fold. First, it fixes the ABI break. Second, this seems like the preferred behavior, if possible. IMO it's the most intuitive implementation. After the LWG issue around array<T, 0> goes through, I suspect the wording will be clarified to make the result of data() != nullptr. And, for people who use array<T, 0> (if there are any) it will be nice not to have to update their code to expect data() to return nullptr and then again to expect data() not to return nullptr.

ldionne requested changes to this revision.Jun 5 2020, 9:17 AM
ldionne added inline comments.
libcxx/include/array
238

I think the problem we had remains -- it's that the union itself isn't trivially copyable even if _Tp is, no? Does libcxx/test/libcxx/containers/sequences/array/triviality.pass.cpp pass with this implementation?

250

I think this is UB: we're only ever constructing the __b member, yet the copy-constructor copies the __t member, which hasn't been initialized. I think this triggers UB when we copy an empty array, which ends up calling this copy constructor on a union member that hasn't been initialized yet.

WDYT?

libcxx/test/std/containers/sequences/array/iterators.pass.cpp
100

This is a great addition, but it doesn't strike me as the right place to put this assertion. Instead, shouldn't that be in libcxx/test/std/containers/sequences/array/array.cons/initialization.pass.cpp?

This revision now requires changes to proceed.Jun 5 2020, 9:17 AM
zoecarver marked 3 inline comments as done.Jun 5 2020, 9:46 AM
zoecarver added inline comments.
libcxx/include/array
238

The union is trivially copyable when it's members are. So, yes, it will be trivially copyable. And the test does pass :)

250

Yes, I think you're right. memcpy is probably the way to go here. I'll use __builtin_memcpy so it's still a constexpr on clang.

libcxx/test/std/containers/sequences/array/iterators.pass.cpp
100

Oops, this was just for my personal sanity check :P

It's probably not a bad idea to keep it, though. The struct is defined in this file so, it seems like this file is the right place for it. I'll move it under the struct definition (I can put another one in initialization.pass.cpp).

ldionne added inline comments.Jun 5 2020, 11:12 AM
libcxx/include/array
250

I mean, even if you use memcpy, it's UB. You can't summon a non trivially copyable object into existence merely by memcpying it, it's still UB to access the object afterwards IIUC.

You can try, but I would expect that Clang is going to error out if you try to take a pointer to __t if it has been "initialized" via memcpy -- which in that case means it hasn't been initialized properly -- in a constexpr context.

libcxx/test/std/containers/sequences/array/iterators.pass.cpp
100

Please only put one in initialization.pass.cpp.

zoecarver marked an inline comment as done.Jun 8 2020, 9:43 AM
zoecarver added inline comments.
libcxx/include/array
250

Is memcpying an object any different than copying a char array of its data? I'
ll give it a try in a constexpr context.

ldionne added inline comments.Jun 8 2020, 10:05 AM
libcxx/include/array
250

No, they are the same.

But my point is that it's not a valid way to copy a *non-trivially-copyable* object. That's the whole point of being non trivially copyable. Does that make sense?

zoecarver abandoned this revision.Jun 8 2020, 10:34 AM
zoecarver marked an inline comment as done.
zoecarver added inline comments.
libcxx/include/array
250

Hmm, I see what you're saying. For what it's worth, up until last week, we would use and copy an array of chars. But, just because we did something wrong for a long time doesn't mean we should continue. I'll close this revision, then. We'll see what LWG says.

ldionne added inline comments.Jun 8 2020, 5:04 PM
libcxx/include/array
250

Can you try it out in a constexpr context with the approach where we memcpy an array of char? Clang must detect UB inside constexpr, so I'm pretty sure it would tell you (and hence that wouldn't be a viable constexpr implementation).

zoecarver reclaimed this revision.Jun 10 2020, 11:56 AM
zoecarver marked an inline comment as done.
zoecarver added inline comments.
libcxx/include/array
250

memcpying the non-active union member address works in a constexpr. BUT we cannot assign or read it (in a constexpr only, we can still assign and read it outside of a constexpr context). This seems OK to me given the alternative is not being able to read or assign it in any context (and having the error be a runtime, not constexpr one).

This revision now requires changes to proceed.Jun 10 2020, 11:56 AM