Page MenuHomePhabricator

[libc++] [LWG3231] Mark "year_month_day_last::day() specification does not cover !ok() values" issue as "Nothing to do", but add assertion.
ClosedPublic

Authored by curdeius on Nov 15 2019, 3:18 PM.

Details

Summary

This LWG issue states that the result of year_month_day_last::day() is implementation defined if ok() is false.
However, from user perspective, calling day() in this situation will lead to a (possibly difficult to find) crash.
Hence, I have added an assertion to warn user at least when assertions are enabled.
I am however not aware of the libc++ stand on the desired behaviour.

Diff Detail

Event Timeline

curdeius created this revision.Nov 15 2019, 3:18 PM
ldionne accepted this revision.May 12 2020, 12:15 PM

I think this is reasonable.

This revision is now accepted and ready to land.May 12 2020, 12:15 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 12 2020, 12:16 PM
This revision now requires review to proceed.May 12 2020, 12:16 PM
ldionne accepted this revision.May 12 2020, 12:16 PM
This revision is now accepted and ready to land.May 12 2020, 12:16 PM
This revision was automatically updated to reflect the committed changes.
curdeius reopened this revision.May 13 2020, 5:02 AM

Reopening revision due to failures in test/libcxx/algorithms/debug_less.pass.cpp.

This revision is now accepted and ready to land.May 13 2020, 5:02 AM
curdeius updated this revision to Diff 263673.May 13 2020, 5:03 AM

Reopen this revision as there was a failure in test/libcxx/algorithms/debug_less.pass.cpp that defined _LIBCPP_ASSERT macro to throw.
The proposed fix includes <chrono> before redefining this macro.

This whole thing of re-defining _LIBCPP_ASSERT doesn't work at all. It's causing all kinds of issues with modules and the test suite too. I think we need to find another solution for *that* problem instead.

Do you know what was the purpose of redefining it?

FYI, there are about 130 occurrences of #define _LIBCPP_ASSERT in tests, but debug_less.pass.cpp is the only place where throw is used (in other cases, exit is called).

Do you know what was the purpose of redefining it?

To test internal aspects of the library. But I think @EricWF and I (at least) both agree that the debug mode of libc++ is not really working right now, and we were talking about rewriting it. This would get rid of most of the other ~130 redefines of _LIBCPP_ASSERT, I think.

To solve just this problem, would it make sense to #define _NOEXCEPT to nothing? I would argue it makes as much sense, since in theory this could have broken long ago on any other method that uses _LIBCPP_ASSERT. WDYT? Does that even work, or do we hit other issues when doing that?

curdeius updated this revision to Diff 264209.May 15 2020, 5:15 AM
  • Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function.

Well, I redefined _NOEXCEPT to nothing, but there are hacks involved.
First of all, libc++ headers include <__config> that defines _NOEXCEPT unconditionally. So I needed to make it conditional.
Secondly, to make this work I replaced noexcept with _NOEXCEPT on day() method in <chrono>.
And yeah, then I defined _NOEXCEPT to nothing, but it seems to me a dangerous endeavour.

ldionne requested changes to this revision.May 15 2020, 6:09 AM

Geez. I really don't like this. All that because we're trying to test something in a way that's broken IMO. I guess I liked your original patch best. Sorry for the back and forth.

libcxx/include/chrono
834

Please position with the other includes

This revision now requires changes to proceed.May 15 2020, 6:09 AM
curdeius updated this revision to Diff 265475.May 21 2020, 5:04 AM
  • Revert "Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function."
  • Reorder include.
curdeius marked an inline comment as done.May 21 2020, 5:04 AM
curdeius updated this revision to Diff 265476.May 21 2020, 5:06 AM
  • Rebase on master.
ldionne accepted this revision.May 21 2020, 12:28 PM

This isn't pretty, but we can do better when we rewrite the debug mode.

This revision is now accepted and ready to land.May 21 2020, 12:28 PM
This revision was automatically updated to reflect the committed changes.

The spec does not allow year_month_day_last::day() to crash. It must not assert. It must not index an array out of bounds. It must not cause undefined behavior.

For example in my example implementation: (2020_y/month{13}/last).day() == 29_d

I recommend a check that month().ok() prior to using it as an index.

The spec does not allow year_month_day_last::day() to crash. It must not assert. It must not index an array out of bounds. It must not cause undefined behavior.

For example in my example implementation: (2020_y/month{13}/last).day() == 29_d

I recommend a check that month().ok() prior to using it as an index.

You are correct, according to http://eel.is/c++draft/time.cal.ymdlast#members-15:

Returns: If ok() is true, returns a day representing the last day of the (year, month) pair represented by *this. Otherwise, the returned value is unspecified.

However, that doesn't strike me as a super useful behavior -- why was it chosen that way? It seems like it's a lot more useful from a user perspective to crash early when asking for the day() of a malformed date?

In the meantime, I'll revert this.

This is part of a larger design philosophy for the entire library. It is not necessarily a logic error to have a date or date component be in a state such that its .ok() returns false. There is an example demonstrating this idea here:

https://github.com/HowardHinnant/date/wiki/Examples-and-Recipes#not_ok_is_ok

Additionally this part of the library is meant to be low-level and fast, without throwing exceptions, asserting, or otherwise terminating. Debug mode for things is great. But down at this level one can't assume that all clients want to pay for debug mode. Some clients may have invariants in their code which make it logically impossible to .ok() == false. Other clients may be using .ok() == false for other purposes (like a nan) as in the above example. And still other clients might find value in layering a higher-level of software on top of <chrono> that adds a checks for .ok(), or for overflowing a duration or time_point, etc. In order to serve all of these clients, <chrono> tries to do as little as possible (exceptions to this rule are parse where everyone wants every check possible).

Additionally my recommendation is not necessarily the highest quality choice. For example returning a !ok() value of day might be desired by your clients. Or if you know your hardware/compiler won't crash by indexing off the end of the array, maybe indexing off the end of the array is a good solution after all. If you've got plenty of space, maybe indexing into an array 256 bytes is an option (assuming month is stored as an unsigned 8 bit byte). Just leave diagnostics to a higher layer of software (that llvm may or may not provide).

There are several places in the spec where I've purposefully chosen unspecified behavior over undefined behavior (and this is one of them). Here is another: http://eel.is/c++draft/time.cal.day.members#1. I want the library to provide an expedient solution with no debugging, while not allowing the compiler to optimize the code out of existence.

ldionne retitled this revision from [libc++] [LWG3321] Mark "year_month_day_last::day() specification does not cover !ok() values" issue as "Nothing to do", but add assertion. to [libc++] [LWG3231] Mark "year_month_day_last::day() specification does not cover !ok() values" issue as "Nothing to do", but add assertion..Jun 9 2020, 9:32 AM

This is part of a larger design philosophy for the entire library. It is not necessarily a logic error to have a date or date component be in a state such that its .ok() returns false. There is an example demonstrating this idea here:

https://github.com/HowardHinnant/date/wiki/Examples-and-Recipes#not_ok_is_ok

Additionally this part of the library is meant to be low-level and fast, without throwing exceptions, asserting, or otherwise terminating. Debug mode for things is great. But down at this level one can't assume that all clients want to pay for debug mode. Some clients may have invariants in their code which make it logically impossible to .ok() == false. Other clients may be using .ok() == false for other purposes (like a nan) as in the above example. And still other clients might find value in layering a higher-level of software on top of <chrono> that adds a checks for .ok(), or for overflowing a duration or time_point, etc. In order to serve all of these clients, <chrono> tries to do as little as possible (exceptions to this rule are parse where everyone wants every check possible).

Additionally my recommendation is not necessarily the highest quality choice. For example returning a !ok() value of day might be desired by your clients. Or if you know your hardware/compiler won't crash by indexing off the end of the array, maybe indexing off the end of the array is a good solution after all. If you've got plenty of space, maybe indexing into an array 256 bytes is an option (assuming month is stored as an unsigned 8 bit byte). Just leave diagnostics to a higher layer of software (that llvm may or may not provide).

There are several places in the spec where I've purposefully chosen unspecified behavior over undefined behavior (and this is one of them). Here is another: http://eel.is/c++draft/time.cal.day.members#1. I want the library to provide an expedient solution with no debugging, while not allowing the compiler to optimize the code out of existence.

Understood. I just created https://reviews.llvm.org/D81477, which I think should solve the problem.