This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Remove VLA from libcxx locale header
ClosedPublic

Authored by DanielMcIntosh-IBM on Jun 2 2021, 2:49 PM.

Details

Summary

The buffer size (__nbuf) in num_put::do_put is currently not an
integral/core constant expression. As a result, __nar is a Variable Length
Array (VLA). VLAs are a GNU extension and not part of the base C++ standard, so
unless there is good reason to do so they probably shouldn't be used in any of
the standard library headers. The call to __iob.flags() is the only thing
keeping __nbuf from being a compile time constant, so the solution here is to
simply err on the side of caution and always allocate a buffer large enough to
fit the base prefix.

Note that, while the base prefix for hex (0x) is slightly longer than the
base prefix for octal (0), this isn't a concern. The difference in the space
needed for the value portion of the string is enough to make up for this.
(Unless we're working with small, oddly sized types such as a hypothetical
uint9_t, the space needed for the value portion in octal is at least 1 more
than the space needed for the value portion in hex).

This PR also adds constexpr to __nbuf to enforce compile time const-ness
going forward.

Diff Detail

Event Timeline

DanielMcIntosh-IBM requested review of this revision.Jun 2 2021, 2:49 PM
DanielMcIntosh-IBM created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 2:49 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Jun 3 2021, 9:16 AM

Note that, while the base prefix for hex (0x) is slightly longer than the
base prefix for octal (0), this isn't a concern. The difference in the space
needed for the value portion of the string is enough to make up for this.

I'm also not concerned since that's the same behaviour as before.

Thanks for the cleanup, LGTM.
I see a lot of build failure on Runtime build node, I assume it's already fixed by D103533. Can you rebase to verify your patch to verify that indeed solve all issues with this patch on that node?

Quuxplusone accepted this revision.Jun 3 2021, 9:39 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/locale
1463–1466

I wonder if this should be _LIBCPP_CONSTEXPR const (which seems redundant) or merely static const (for C++03 compatibility) or merely const. But I guess as long as it compiles it's fine any way.

This revision is now accepted and ready to land.Jun 3 2021, 9:39 AM
libcxx/include/locale
1463–1466

const and static const by themselves wont force __nbuf to be a compile-time constant. If __nbuf is not constant-initialized, then the __nbuf expression used to set the size of __nar will silently stop being an integral constant expression (it will stop satisfying exception a to rule 9 of what isn't a core constant expression). This is why we were able to use __iob.flags() in the initializer without generating any errors.

constexpr by itself would be enough, but can't be used prior to C++11, so we need to use _LIBCPP_CONSTEXPR. However, we still can't remove the const because pre-c++11, that would leave __nbuf with neither const nor constexpr, which would again stop __nbuf from satisfying exception a to rule 9.

While the _LIBCPP_CONSTEXPR isn't strictly necessary, it prevents future issues like this one by generating an error if __nbuf isn't a compile-time constant.

DanielMcIntosh-IBM retitled this revision from Remove VLA from libcxx locale header to [libcxx] Remove VLA from libcxx locale header.Jun 7 2021, 11:57 AM

Just FYI when we approve with the condition the CI is green after a rebase, you're free to land the patch after the CI passes.
So feel free to land the patch, or do you need somebody to commit it on your behalf?
If so please provide your name and e-mail address so we commit it on your behalf.

ldionne accepted this revision.Jun 8 2021, 7:38 AM