diff mbox

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

Message ID 20140516110931.GA29145@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely May 16, 2014, 11:09 a.m. UTC 
On 15/05/14 22:54 -0400, Ed Smith-Rowland wrote:
>On 05/15/2014 03:03 PM, Jonathan Wakely wrote:
>>Here's a finished patch to simplify <bits/parse_numbers.h>
>>
>>Tested x86_64-linux. Ed, any objection to this version?
>>
>This looks great, thanks!

I committed that to trunk, I'll put it on the 4.9 branch too.

>Having done that should we actually stop using it as suggested in the 
>bug trail? ;-)

I was going to do that, then realised that there's a defect in the
standard where it requires overflow in duration integer literals to be
diagnosed. That's only possible with literal operator templates, so I
think we should keep your _Parse_int code, but apply the attached
change to detect overflow.

As the TODO comment says, it should be sufficient to simply
instantiate integral_constant<_Rep, _Val::value> to give a diagnostic
when _Rep{_Value::value} is narrowing, but GCC only gives a warning
for it, and that's suppressed in a system header, so I do an explicit
static_assert.  That could be replaced with ...

#pragma GCC diagnostic push
#pragma GCC diagnostic error "-Woverflow"
#pragma GCC diagnostic error "-Wsystem-headers"
    template<typename _Dur, char... _Digits>
      constexpr _Dur __check_overflow()
      {
        using _Val = __parse_int::_Parse_int<_Digits...>;
        using _Rep = typename _Dur::rep;
        return _Dur{integral_constant<_Rep, _Val::value>::value};
      }
#pragma GCC diagnostic pop

... but I have plans to do that sort of thing more widely, which I'll
deal with another time as part of https://gcc.gnu.org/PR50871 and/or
https://gcc.gnu.org/PR58876 (what do other people think about using
diagnostic pragmas to locally re-enable diagnostics in our headers?)

Tested x86_64-linux, committed to trunk.
diff mbox

Patch

commit 361c9b79e0b1c7f2435f5b0b369a139b216dee90
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri May 16 10:31:38 2014 +0100

    	* include/bits/parse_numbers.h (__parse_int::_Number_help): Check for
    	overflow.
    	* include/std/chrono (chrono_literals::__select_type::_Select_type):
    	Remove.
    	(chrono_literals::_Checked_integral_constant): Define.
    	Simplify UDL operator templates and check for overflow.
    	* testsuite/20_util/duration/literals/range.cc: New.

diff --git a/libstdc++-v3/include/bits/parse_numbers.h b/libstdc++-v3/include/bits/parse_numbers.h
index 0a42381a..a29d127 100644
--- a/libstdc++-v3/include/bits/parse_numbers.h
+++ b/libstdc++-v3/include/bits/parse_numbers.h
@@ -193,6 +193,7 @@  namespace __parse_int
 				  _Pow / (_Base * __valid_digit::value),
 				  _Digs...>;
       using type = __ull_constant<_Pow * __digit::value + __next::type::value>;
+      static_assert((type::value / _Pow) == __digit::value, "overflow");
     };
 
   template<unsigned _Base, unsigned long long _Pow, char _Dig>
diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono
index b114e02..39ad5e3 100644
--- a/libstdc++-v3/include/std/chrono
+++ b/libstdc++-v3/include/std/chrono
@@ -787,117 +787,79 @@  _GLIBCXX_END_NAMESPACE_VERSION
   inline namespace chrono_literals
   {
 
-    namespace __select_type
-    {
-
-      using namespace __parse_int;
-
-      template<unsigned long long _Val, typename _Dur>
-	struct _Select_type
-	: conditional<
-	    _Val <= static_cast<unsigned long long>
-		      (numeric_limits<typename _Dur::rep>::max()),
-	    _Dur, void>
-	{
-	  static constexpr typename _Select_type::type
-	    value{static_cast<typename _Select_type::type>(_Val)};
-	};
-
-      template<unsigned long long _Val, typename _Dur>
-	constexpr typename _Select_type<_Val, _Dur>::type
-	_Select_type<_Val, _Dur>::value;
+    template<typename _Rep, unsigned long long _Val>
+      struct _Checked_integral_constant
+      : integral_constant<_Rep, static_cast<_Rep>(_Val)>
+      {
+	static_assert(_Checked_integral_constant::value > 0
+		      && _Checked_integral_constant::value == _Val,
+		      "literal value cannot be represented by duration type");
+      };
 
-    } // __select_type
+    template<typename _Dur, char... _Digits>
+      constexpr _Dur __check_overflow()
+      {
+	using _Val = __parse_int::_Parse_int<_Digits...>;
+	using _Rep = typename _Dur::rep;
+	// TODO: should be simply integral_constant<_Rep, _Val::value>
+	// but GCC doesn't reject narrowing conversions to _Rep.
+	using _CheckedVal = _Checked_integral_constant<_Rep, _Val::value>;
+	return _Dur{_CheckedVal::value};
+      }
 
     constexpr chrono::duration<long double, ratio<3600,1>>
     operator""h(long double __hours)
     { return chrono::duration<long double, ratio<3600,1>>{__hours}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::hours>::type
+      constexpr chrono::hours
       operator""h()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::hours>::value;
-      }
+      { return __check_overflow<chrono::hours, _Digits...>(); }
 
     constexpr chrono::duration<long double, ratio<60,1>>
     operator""min(long double __mins)
     { return chrono::duration<long double, ratio<60,1>>{__mins}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::minutes>::type
+      constexpr chrono::minutes
       operator""min()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::minutes>::value;
-      }
+      { return __check_overflow<chrono::minutes, _Digits...>(); }
 
     constexpr chrono::duration<long double>
     operator""s(long double __secs)
     { return chrono::duration<long double>{__secs}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::seconds>::type
+      constexpr chrono::seconds
       operator""s()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::seconds>::value;
-      }
+      { return __check_overflow<chrono::seconds, _Digits...>(); }
 
     constexpr chrono::duration<long double, milli>
     operator""ms(long double __msecs)
     { return chrono::duration<long double, milli>{__msecs}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::milliseconds>::type
+      constexpr chrono::milliseconds
       operator""ms()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::milliseconds>::value;
-      }
+      { return __check_overflow<chrono::milliseconds, _Digits...>(); }
 
     constexpr chrono::duration<long double, micro>
     operator""us(long double __usecs)
     { return chrono::duration<long double, micro>{__usecs}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::microseconds>::type
+      constexpr chrono::microseconds
       operator""us()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::microseconds>::value;
-      }
+      { return __check_overflow<chrono::microseconds, _Digits...>(); }
 
     constexpr chrono::duration<long double, nano>
     operator""ns(long double __nsecs)
     { return chrono::duration<long double, nano>{__nsecs}; }
 
     template <char... _Digits>
-      constexpr typename
-      __select_type::_Select_type<__select_int::_Select_int<_Digits...>::value,
-			     chrono::nanoseconds>::type
+      constexpr chrono::nanoseconds
       operator""ns()
-      {
-	return __select_type::_Select_type<
-			  __select_int::_Select_int<_Digits...>::value,
-			  chrono::nanoseconds>::value;
-      }
+      { return __check_overflow<chrono::nanoseconds, _Digits...>(); }
 
   } // inline namespace chrono_literals
   } // inline namespace literals
diff --git a/libstdc++-v3/testsuite/20_util/duration/literals/range.cc b/libstdc++-v3/testsuite/20_util/duration/literals/range.cc
new file mode 100644
index 0000000..c005df6
--- /dev/null
+++ b/libstdc++-v3/testsuite/20_util/duration/literals/range.cc
@@ -0,0 +1,31 @@ 
+// { dg-do compile }
+// { dg-options "-std=gnu++1y" }
+
+// Copyright (C) 2014 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <chrono>
+
+void
+test01()
+{
+  using namespace std::literals::chrono_literals;
+
+  // std::numeric_limits<int64_t>::max() == 9223372036854775807;
+  auto h = 9223372036854775808h;
+  // { dg-error "cannot be represented" "" { target *-*-* } 794 }
+}