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

[curdeius]

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.

[curdeius]

Ping. :)

[ldionne]

I think this is reasonable.

[curdeius]

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

[curdeius]

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.

[ldionne]

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.

[curdeius]

Do you know what was the purpose of redefining it?

[curdeius]

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).

[ldionne]

In D70346#2034133, @curdeius wrote:

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]

• Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function.

[curdeius]

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]

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

[curdeius]

• Revert "Define _NOEXCEPT to nothing for debug_less tests to avoid throwing in noexcept function."
• Reorder include.

[curdeius]

• Rebase on master.

[ldionne]

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

[howard.hinnant]

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.

[ldionne]

In D70346#2082298, @howard.hinnant wrote:

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.

[howard.hinnant]

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]

In D70346#2082458, @howard.hinnant wrote:

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.

libcxx/include/chrono

 constexpr chrono::year                                  operator ""y(unsigned long long y) noexcept; // C++20
 }  // chrono_literals
 }  // literals
 
 }  // std
 */
 
 #include <__config>
+#include <__debug>
 #include <ctime>
 #include <type_traits>
 #include <ratio>
 #include <limits>
 #include <version>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 #ifndef _LIBCPP_CXX03_LANG
 _LIBCPP_BEGIN_NAMESPACE_FILESYSTEM

 {
     constexpr chrono::day __d[] =
     {
         chrono::day(31), chrono::day(28), chrono::day(31),
         chrono::day(30), chrono::day(31), chrono::day(30),
         chrono::day(31), chrono::day(31), chrono::day(30),
         chrono::day(31), chrono::day(30), chrono::day(31)
     };
+    _LIBCPP_ASSERT(ok(), "year_month_day_last::day(): year_month_day_last is invalid");
     return month() != February || !__y.is_leap() ?
         __d[static_cast<unsigned>(month()) - 1] : chrono::day{29};
 }
 
 inline constexpr
 bool operator==(const year_month_day_last& __lhs, const year_month_day_last& __rhs) noexcept
 { return __lhs.year() == __rhs.year() && __lhs.month_day_last() == __rhs.month_day_last(); }
 

libcxx/test/libcxx/algorithms/debug_less.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: no-exceptions
 
 // <algorithm>
 
 // template <class _Compare> struct __debug_less
 
 // __debug_less checks that a comparator actually provides a strict-weak ordering.
 
+#include <chrono> // Include before defining _LIBCPP_ASSERT: cannot throw in a function marked noexcept.
+
 struct DebugException {};
 
+#ifdef _LIBCPP_ASSERT
+#undef _LIBCPP_ASSERT
+#endif
 #define _LIBCPP_DEBUG 0
 #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : throw ::DebugException())
 
 #include <algorithm>
 #include <iterator>
 #include <cassert>
 
 #include "test_macros.h"

libcxx/www/cxx2a_status.html

-->
   <tr><td><a href="https://wg21.link/LWG3198">3198</a></td><td>Bad constraint on <tt>std::span::span()</tt></td><td>Cologne</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG3199">3199</a></td><td><tt>istream &gt;&gt; bitset&lt;0&gt;</tt> fails</td><td>Cologne</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3202">3202</a></td><td>P0318R1 was supposed to be revised</td><td>Cologne</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG3206">3206</a></td><td><tt>year_month_day</tt> conversion to <tt>sys_days</tt> uses not-existing member function</td><td>Cologne</td><td>Complete</td></tr>
   <tr><td><a href="https://wg21.link/LWG3208">3208</a></td><td><tt>Boolean</tt>'s expression requirements are ordered inconsistently</td><td>Cologne</td><td><i>Nothing to do</i></td></tr>
   <tr><td><a href="https://wg21.link/LWG3209">3209</a></td><td>Expression in <tt>year::ok()</tt> returns clause is ill-formed</td><td>Cologne</td><td>Complete</td></tr>
 
   <tr><td></td><td></td><td></td><td></td></tr>
-  <tr><td><a href="https://wg21.link/LWG3231">3231</a></td><td><tt>year_month_day_last::day</tt> specification does not cover <tt>!ok()</tt> values</td><td>Belfast</td><td></td></tr>
+  <tr><td><a href="https://wg21.link/LWG3231">3231</a></td><td><tt>year_month_day_last::day</tt> specification does not cover <tt>!ok()</tt> values</td><td>Belfast</td><td><i>Nothing to do</i></td></tr>
   <tr><td><a href="https://wg21.link/LWG3225">3225</a></td><td><tt>zoned_time</tt> converting constructor shall not be <tt>noexcept</tt></td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3190">3190</a></td><td><tt>std::allocator::allocate</tt> sometimes returns too little storage</td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3218">3218</a></td><td>Modifier for <tt>%d</tt> parse flag does not match POSIX and <tt>format</tt> specification</td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3224">3224</a></td><td><tt>zoned_time</tt> constructor from <tt>TimeZonePtr</tt> does not specify initialization of <tt>tp_</tt></td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3230">3230</a></td><td>Format specifier <tt>%y/%Y</tt> is missing locale alternative versions</td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3232">3232</a></td><td>Inconsistency in <tt>zoned_time</tt> deduction guides</td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3222">3222</a></td><td>P0574R1 introduced preconditions on non-existent parameters</td><td>Belfast</td><td></td></tr>
   <tr><td><a href="https://wg21.link/LWG3221">3221</a></td><td>Result of <tt>year_month</tt> arithmetic with <tt>months</tt> is ambiguous</td><td>Belfast</td><td></td></tr>