[Bug libstdc++/61166] New: overflow when parse number in std::duration operator""

[Bug libstdc++/61166] New: overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

            Bug ID: 61166
           Summary: overflow when parse number in std::duration operator""
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kan.liu.229 at gmail dot com

overflow may occur when using a number whose size exceeds sizeof(unsigned) in
chrono_literals. Because the parse classes in include/bits/parse_number.h just
use *unsigned* for member type. However, it should accept *unsigned long long*
I think. This issue also exists in trunk I think.

#include <chrono>

using namespace std;
using namespace std::chrono;

int main() {
    // std::numeric_limits<unsigned>::max() == 4294967295
    cout << 429496729510 << endl;
    cout << (429496729510h).count() << endl;
    return 0;
}

And the result is

429496729510 
926648584

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[paolo.carlini at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|kan.liu.229 at gmail dot com       |3dw4rd at verizon dot net

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> ---
Ed, please have a look to this one too.

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[emsr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #2 from emsr at gcc dot gnu.org ---
Created attachment 32789
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32789&action=edit
Patch to parse_nmber to make it unsigned long long all over.

Works on x86_64-linux.

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #3 from Kan Liu <kan.liu.229 at gmail dot com> ---
(In reply to emsr from comment #2)
> Created attachment 32789 [details]
> Patch to parse_nmber to make it unsigned long long all over.
> 
> Works on x86_64-linux.

Yeah, it works. Thank you!

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #4 from Kan Liu <kan.liu.229 at gmail dot com> ---
btw, is it really necessary to use functionality in parse_number.h to parse
manually? What *parse_number* has done is no more than the general
*operator""(unsigned long long)*, and it just enables *0b* and *0B* prefix for
the numbers. I think *operator""(unsigned long long)* is quite enough for this.

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[3dw4rd at verizon dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #5 from Ed Smith-Rowland <3dw4rd at verizon dot net> ---
Created attachment 32792
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32792&action=edit
Better patch with test case.

2014-05-13  Ed Smith-Rowland  <3dw4rd@verizon.net>

        PR libstdc++/61166
        * include/bits/parse_number.h: Replace 'unsigned'
        with 'unsigned long long'.
        * testsuite/20_util/duration/literals/pr61166.cc: New.

I'll send this to gcc-patches when my testing is complete

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #7 from Kan Liu <kan.liu.229 at gmail dot com> ---
(In reply to emsr from comment #6)
> Something like parse_number was in the original doc as an implementation
> example.  The idea was to select the smallest integral type that could
> accommodate the number string with.  This is done with _Select_int. 
> _Select_type picks the most space-efficient duration ratio type using
> _Select_int.

Yeah, but it only helps saving space when compiling time for now. For now
implementation, _Select_type takes the default chrono::hour (and other time
units), which uses *int64_t* as its underlying representation. So, it doesn't
save space I think.

Take *ms* as example
Implementation now:

    template <char... _Digits>
      constexpr typename
      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
                             chrono::milliseconds>::type
      operator""ms()
      {
        return __select_type::_Select_type<
                          __select_int::_Select_int<_Digits...>::value,
                          chrono::milliseconds>::value;
      }

space-efficient version (maybe), get the type from _Select_int:

template <char... _Digits>
constexpr typename
chrono::duration<decltype(__select_int::_Select_int<_Digits...>::value), milli>
operator""_ms()
{
    typedef typename __select_int::_Select_int<_Digits...> __selected;
    typedef typename chrono::duration<decltype(__selected::value), milli>
__duration_type;
    return __duration_type {__selected::value};
}

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
That's correct, and the standard is clear that operator""ms(unsigned long long)
is required to return chrono::milliseconds, not some other equivalent duration
with a different Rep.

So I think the question in comment 4 is valid: why do we bother with these
templates at all?

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-05-14
     Ever confirmed|0                           |1

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I can see one good reason to implement those operators as templates: it allows
us to check if the literal value overflows the duration::rep type, which is
required by the standard but impossible to implement without using templates.

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #10 from Kan Liu <kan.liu.229 at gmail dot com> ---
(In reply to Jonathan Wakely from comment #9)
> I can see one good reason to implement those operators as templates: it
> allows us to check if the literal value overflows the duration::rep type,
> which is required by the standard but impossible to implement without using
> templates.

_Select_type already does the overflow check, so *template implemented
operators* is still redundant I think. Since the standard units (std::hours,
milliseconds ...) requires the duration::rep to be *int64_t*, there's no need
to do too much check.

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Kan Liu from comment #10)
> _Select_type already does the overflow check, so *template implemented
> operators* is still redundant I think.

You can't use _Select_type on a literal operator that is not a template.

> Since the standard units (std::hours,
> milliseconds ...) requires the duration::rep to be *int64_t*,

No it doesn't, only chrono::nanoseconds is required to have a 64-bit
representation. chrono::hours and chrono::minutes are allowed to have fewer
than 32 bits ("at least 23 bits" and "at least 29 bits" respectively).

> there's no
> need to do too much check.

Not all values of uint64_t fit in int64_t

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[kan.liu.229 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #12 from Kan Liu <kan.liu.229 at gmail dot com> ---
(In reply to Jonathan Wakely from comment #11)
> (In reply to Kan Liu from comment #10)
> > _Select_type already does the overflow check, so *template implemented
> > operators* is still redundant I think.
> 
> You can't use _Select_type on a literal operator that is not a template.
> 
> > Since the standard units (std::hours,
> > milliseconds ...) requires the duration::rep to be *int64_t*,
> 
> No it doesn't, only chrono::nanoseconds is required to have a 64-bit
> representation. chrono::hours and chrono::minutes are allowed to have fewer
> than 32 bits ("at least 23 bits" and "at least 29 bits" respectively).
> 
> > there's no
> > need to do too much check.
> 
> Not all values of uint64_t fit in int64_t

Wow... I see

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Author: redi
Date: Fri May 16 09:30:57 2014
New Revision: 210511

URL: http://gcc.gnu.org/viewcvs?rev=210511&root=gcc&view=rev
Log:
2014-05-15  Ed Smith-Rowland  <3dw4rd@verizon.net>
        Jonathan Wakely  <jwakely@redhat.com>

    PR libstdc++/61166
    * include/bits/parse_numbers.h: Use integral_constant to remove
    duplication and simplify.
    * testsuite/20_util/duration/literals/61166.cc: New.

Added:
    trunk/libstdc++-v3/testsuite/20_util/duration/literals/61166.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/parse_numbers.h

[Bug libstdc++/61166] overflow when parse number in std::duration operator""

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61166

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.10.0

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk - I'm not planning to fix this for 4.9 unless it's really
needed.