| Message ID | 20231112013352.19885-1-cassio.neri@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix UB in weekday::weekday(sys_days) and add test. | expand |
On Sun, 12 Nov 2023 at 01:34, Cassio Neri <cassio.neri@gmail.com> wrote: > > The following has undefined behaviour (signed overflow) [1]: > weekday max{sys_days{days{numeric_limits<days::rep>::max()}}}; > > The issue is in this line when __n is very large and __n + 4 overflows: > return weekday(__n >= -4 ? (__n + 4) % 7 : (__n + 5) % 7 + 6); > > In addition to fixing this bug, the new implementation makes the compiler emit > shorter and branchless code for x86-64 and ARM [2]. > > [1] https://godbolt.org/z/1s5bv7KfT > [2] https://godbolt.org/z/zKsabzrhs > > libstdc++-v3/ChangeLog: > > * include/std/chrono: Fix weekday::_S_from_days > * testsuite/std/time/weekday/1.cc: Add test for overflow. > --- > > Good for trunk? Pushed to trunk, thanks. I think this one is worth backporting too. > > libstdc++-v3/include/std/chrono | 11 +++++++++-- > libstdc++-v3/testsuite/std/time/weekday/1.cc | 9 +++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono > index 10e868e5a03..c00dd133173 100644 > --- a/libstdc++-v3/include/std/chrono > +++ b/libstdc++-v3/include/std/chrono > @@ -930,8 +930,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > static constexpr weekday > _S_from_days(const days& __d) > { > - auto __n = __d.count(); > - return weekday(__n >= -4 ? (__n + 4) % 7 : (__n + 5) % 7 + 6); > + using _Rep = days::rep; > + using _URep = make_unsigned_t<_Rep>; > + const auto __n = __d.count(); > + const auto __m = static_cast<_URep>(__n); > + > + // 1970-01-01 (__n = 0, __m = 0 ) -> Thursday (4) > + // 1969-31-12 (__n = -1, __m = _URep(-1)) -> Wednesday (3) > + const auto __offset = __n >= 0 ? _URep(4) : 3 - _URep(-1) % 7 - 7; > + return weekday((__m + __offset) % 7); > } > > public: > diff --git a/libstdc++-v3/testsuite/std/time/weekday/1.cc b/libstdc++-v3/testsuite/std/time/weekday/1.cc > index 00278c8b01c..e89fca47d4b 100644 > --- a/libstdc++-v3/testsuite/std/time/weekday/1.cc > +++ b/libstdc++-v3/testsuite/std/time/weekday/1.cc > @@ -20,6 +20,7 @@ > // Class template day [time.cal.weekday] > > #include <chrono> > +#include <limits> > > constexpr void > constexpr_weekday() > @@ -37,6 +38,14 @@ constexpr_weekday() > static_assert(weekday{3}[2].weekday() == weekday{3}); > static_assert(weekday{3}[last].weekday() == weekday{3}); > > + // Test for UB (overflow). > + { > + using rep = days::rep; > + using std::numeric_limits; > + constexpr weekday max{sys_days{days{numeric_limits<rep>::max()}}}; > + constexpr weekday min{sys_days{days{numeric_limits<rep>::min()}}}; > + } > + > static_assert(weekday{sys_days{1900y/January/1}} == Monday); > static_assert(weekday{sys_days{1970y/January/1}} == Thursday); > static_assert(weekday{sys_days{2020y/August/21}} == Friday); > -- > 2.41.0 >
diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 10e868e5a03..c00dd133173 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -930,8 +930,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static constexpr weekday _S_from_days(const days& __d) { - auto __n = __d.count(); - return weekday(__n >= -4 ? (__n + 4) % 7 : (__n + 5) % 7 + 6); + using _Rep = days::rep; + using _URep = make_unsigned_t<_Rep>; + const auto __n = __d.count(); + const auto __m = static_cast<_URep>(__n); + + // 1970-01-01 (__n = 0, __m = 0 ) -> Thursday (4) + // 1969-31-12 (__n = -1, __m = _URep(-1)) -> Wednesday (3) + const auto __offset = __n >= 0 ? _URep(4) : 3 - _URep(-1) % 7 - 7; + return weekday((__m + __offset) % 7); } public: diff --git a/libstdc++-v3/testsuite/std/time/weekday/1.cc b/libstdc++-v3/testsuite/std/time/weekday/1.cc index 00278c8b01c..e89fca47d4b 100644 --- a/libstdc++-v3/testsuite/std/time/weekday/1.cc +++ b/libstdc++-v3/testsuite/std/time/weekday/1.cc @@ -20,6 +20,7 @@ // Class template day [time.cal.weekday] #include <chrono> +#include <limits> constexpr void constexpr_weekday() @@ -37,6 +38,14 @@ constexpr_weekday() static_assert(weekday{3}[2].weekday() == weekday{3}); static_assert(weekday{3}[last].weekday() == weekday{3}); + // Test for UB (overflow). + { + using rep = days::rep; + using std::numeric_limits; + constexpr weekday max{sys_days{days{numeric_limits<rep>::max()}}}; + constexpr weekday min{sys_days{days{numeric_limits<rep>::min()}}}; + } + static_assert(weekday{sys_days{1900y/January/1}} == Monday); static_assert(weekday{sys_days{1970y/January/1}} == Thursday); static_assert(weekday{sys_days{2020y/August/21}} == Friday);
The following has undefined behaviour (signed overflow) [1]: weekday max{sys_days{days{numeric_limits<days::rep>::max()}}}; The issue is in this line when __n is very large and __n + 4 overflows: return weekday(__n >= -4 ? (__n + 4) % 7 : (__n + 5) % 7 + 6); In addition to fixing this bug, the new implementation makes the compiler emit shorter and branchless code for x86-64 and ARM [2]. [1] https://godbolt.org/z/1s5bv7KfT [2] https://godbolt.org/z/zKsabzrhs libstdc++-v3/ChangeLog: * include/std/chrono: Fix weekday::_S_from_days * testsuite/std/time/weekday/1.cc: Add test for overflow. --- Good for trunk? libstdc++-v3/include/std/chrono | 11 +++++++++-- libstdc++-v3/testsuite/std/time/weekday/1.cc | 9 +++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) -- 2.41.0