Bug 69905 - Digit separators break literal operators
Summary: Digit separators break literal operators
Status: RESOLVED DUPLICATE of bug 84671
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-22 20:49 UTC by Alisdair Meredith
Modified: 2018-05-17 10:07 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-10-26 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alisdair Meredith 2016-02-22 20:49:50 UTC
The following code fails to compile when the macro is defined:

#include <chrono>
using namespace std::literals;

#if defined SHOW_BUG
auto x = 1'23s
#else
auto x = 123s;
#endif

I /think/ this is a problem in the front-end, rather than the library, as the digit separator should not be visible to the library call.
Comment 1 Marek Polacek 2016-02-25 09:58:50 UTC
It compiles for me with GCC 6 (and gnu++14).
Comment 2 Tad Ashlock 2016-08-11 18:49:35 UTC
I can confirm this problem, and add a little more detail.  Here's the code I used:

#include <chrono>
using namespace std::chrono_literals;
int main ()
{
    auto const test1 = 1100s;
    auto const test2 = 1'000s;
    auto const test3 = 1'100s;
    return 0;
}

The test1 and test2 assignments compile, indicating that 1100 seconds isn't too large of a number, and 1'000 is accepted.  It turns out that a non-0 digit following the separator is causing the problem.

But test3 fails with the message:

g++ -std=c++14 test.cc        
In file included from /opt/tools/gcc/6.1.0/include/c++/6.1.0/chrono:42:0,
                 from test.cc:1:
/opt/tools/gcc/6.1.0/include/c++/6.1.0/bits/parse_numbers.h: In instantiation of ‘struct std::__parse_int::_Number_help<10u, 100ull, '\'', '1', '0', '0'>’:
/opt/tools/gcc/6.1.0/include/c++/6.1.0/bits/parse_numbers.h:195:57:   required from ‘struct std::__parse_int::_Number_help<10u, 1000ull, '1', '\'', '1', '0', '0'>’
/opt/tools/gcc/6.1.0/include/c++/6.1.0/bits/parse_numbers.h:208:12:   required from ‘struct std::__parse_int::_Number<10u, '1', '\'', '1', '0', '0'>’
/opt/tools/gcc/6.1.0/include/c++/6.1.0/bits/parse_numbers.h:248:12:   required from ‘struct std::__parse_int::_Parse_int<'1', '\'', '1', '0', '0'>’
/opt/tools/gcc/6.1.0/include/c++/6.1.0/chrono:808:67:   required from ‘constexpr _Dur std::literals::chrono_literals::__check_overflow() [with _Dur = std::chrono::duration<long int>; char ..._Digits = {'1', '\'', '1', '0', '0'}]’
/opt/tools/gcc/6.1.0/include/c++/6.1.0/chrono:837:61:   required from ‘constexpr std::chrono::seconds std::literals::chrono_literals::operator""s() [with char ..._Digits = {'1', '\'', '1', '0', '0'}; std::chrono::seconds = std::chrono::duration<long int>]’
test.cc:7:24:   required from here
/opt/tools/gcc/6.1.0/include/c++/6.1.0/bits/parse_numbers.h:196:7: error: static assertion failed: integer literal does not fit in unsigned long long
       static_assert((type::value / _Pow) == __digit::value,
       ^~~~~~~~~~~~~
Comment 3 aaron.mcdaid 2016-10-26 14:34:56 UTC
I think this could be fixed by defining the missing overload of `operator""s` for `unsigned long long`.

As I understand it, the standard requires two overloads, `long double` and `unsigned long long`. However, only the latter is present in gcc-6.2.0. The `chrono` file is missing `unsigned unsigned long`, but has the following instead:

      template <char... _Digits>
      constexpr chrono::seconds
      operator""s()
      { return __check_overflow<chrono::seconds, _Digits...>();

This appears to be failing to interpret the separator correctly.

(Apologies if I have misinterpreted, this is my first time looking in the chrono header!)
Comment 4 TC 2016-10-26 14:58:07 UTC
>   template<unsigned _Base, unsigned long long _Pow, char _Dig, char... _Digs>
>   struct _Number_help
>   {
>     using __digit = _Digit<_Base, _Dig>;
>     using __valid_digit = typename __digit::__valid;
>     using __next = _Number_help<_Base,
>				  __valid_digit::value ? _Pow / _Base : _Pow,
>				  _Digs...>;
>     using type = __ull_constant<_Pow * __digit::value + __next::type::value>;
>     static_assert((type::value / _Pow) == __digit::value,
>		    "integer literal does not fit in unsigned long long");
>   };

The issue is that when __valid_digit is false_type (i.e., we have a digit separator), the static_assert check should not be done. In this case, __next is computed using the same _Pow, and __digit::value is zero, which means that it currently fires whenever the next digit is nonzero.

I think the fix is simply

     static_assert(!__valid_digit::value || (type::value / _Pow) == __digit::value,
                   "integer literal does not fit in unsigned long long");
Comment 5 Jonathan Wakely 2016-10-26 15:03:20 UTC
Yes that looks right, we didn't consider digit separators in that function template.

It's by design that we use that rather than an operator""s(unsigned long long) because otherwise it's not possible to check for overflow, see https://wg21.link/lwg2383
Comment 6 Ed Smith-Rowland 2016-10-26 19:38:00 UTC
If no one beats me to it I'll post a patch in an hour after testing.
I knew I added separators and I thought I had tested it...

Ed
Comment 7 andreas.molzer 2018-05-16 23:07:53 UTC
Leaving out the overload for operator""s(long double) will silently break valid code. Consider:

#include <chrono>
int main() {
    using std::chrono_literals;
    auto time = operator""s(0x8000'0000'0000'0000'0000'0000ULL);
}

According to standard, this should select the overload with unsigned long long argument, which does not exist.  It will instead silently perform a numeric conversion to long double, resulting in loss of accuracy on targets where gcc does not use extended 80-bit double.  Any other integer arguments, which should lead to overload resolution failure, will also be double durations albeit with less immediately catastrophic results. 

However, that doesn't mean the long term effects are no issue.  Consider that the type of time.count() also unexpectedly resolves as long double.  Or even worse that assigning decltype(time)::max() to unsigned long long is undefined behaviour.

It might be more fitting to open the missing declarations as a separately tracked bug, improving visibility of the standard issue as well.
Comment 8 Jonathan Wakely 2018-05-17 09:27:04 UTC
The testcase in comment 2 (the only one in the bug that's actually correct!) works now, fixed by r258157 for Bug 84671.

The problem in comment 7 still exists, the corrected testcase is:

#include <chrono>
int main() {
    using namespace std::chrono_literals;
    auto time = operator""s(0x8000'0000'0000'0000'0000'0000ULL);
    static_assert(std::is_same<decltype(time)::rep, unsigned long long>(), "");
}
Comment 9 Jonathan Wakely 2018-05-17 10:07:01 UTC
(In reply to andreas.molzer from comment #7)
> Leaving out the overload for operator""s(long double) will silently break

We provide that overload though. What do you mean?

> valid code. Consider:
> 
> #include <chrono>
> int main() {
>     using std::chrono_literals;
>     auto time = operator""s(0x8000'0000'0000'0000'0000'0000ULL);
> }
> 
> According to standard, this should select the overload with unsigned long
> long argument, which does not exist.

No, according to the standard this is ill-formed because the value doesn't fit in an unsigned long long (assuming 64 bits). GCC accepts it unless you use -pedantic-errors, but this isn't valid code.

>  It will instead silently perform a
> numeric conversion to long double, resulting in loss of accuracy on targets
> where gcc does not use extended 80-bit double.  Any other integer arguments,
> which should lead to overload resolution failure, will also be double
> durations albeit with less immediately catastrophic results. 

Again, this doesn't silently break valid code, because what you're doing should be ill-formed according to the standard. So not valid code.

> However, that doesn't mean the long term effects are no issue.  Consider
> that the type of time.count() also unexpectedly resolves as long double.  Or
> even worse that assigning decltype(time)::max() to unsigned long long is
> undefined behaviour.
> 
> It might be more fitting to open the missing declarations as a separately
> tracked bug, improving visibility of the standard issue as well.

Feel free to open a new bug (but this is not the place to report problems in the standard, see https://cplusplus.github.io/LWG/lwg-active.html#submit_issue for that).

I'm going to close this one as a dup of Bug 84671 because I don't think there's any compiler bug here. The original problem was due to the library not handling digit separators in its UDL operators, which is fixed.

*** This bug has been marked as a duplicate of bug 84671 ***