[chrono] Possible bug in boost::chrono::process_real_cpu_clock::now() under Windows 32bits
Hi, As of 1.48 Boost.Chrono contains code below for boost::chrono::process_real_cpu_clock::now() (boost\chrono\detail\inlined\win\process_cpu_clocks.hpp) clock_t c = ::clock(); /* ... */ return time_point( duration(c*(1000000000l/CLOCKS_PER_SEC)) ); duration::rep is int64/nanoseconds, clock_t is long. This is VS2008, Win XP 32bits. I think "c" should be cast to duration::rep before being multiplied. I randomly get negative time_point and this seems to be fixed by replacing "c" with "((duration::rep) (c))" in duration constructor : duration( ((duration::rep) (c)) *(1000000000l/CLOCKS_PER_SEC)) Can someone confirm my view and that the fix is correct ? Regards, Ivan
On Jan 5, 2012, at 10:30 AM, ivan.lelann@free.fr wrote:
Hi,
As of 1.48 Boost.Chrono contains code below for boost::chrono::process_real_cpu_clock::now() (boost\chrono\detail\inlined\win\process_cpu_clocks.hpp)
clock_t c = ::clock(); /* ... */ return time_point( duration(c*(1000000000l/CLOCKS_PER_SEC)) );
duration::rep is int64/nanoseconds, clock_t is long. This is VS2008, Win XP 32bits.
I think "c" should be cast to duration::rep before being multiplied. I randomly get negative time_point and this seems to be fixed by replacing "c" with "((duration::rep) (c))" in duration constructor :
duration( ((duration::rep) (c)) *(1000000000l/CLOCKS_PER_SEC))
Can someone confirm my view and that the fix is correct ?
Your fix looks correct to me. I'm not positive what value CLOCKS_PER_SEC has on your platform. But here is a reformulation of your solution that might be a little safer (depending on what CLOCKS_PER_SEC actually is): typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; return time_point( duration(static_cast<rep>(c)*R::num/R::den) ); Using giga avoids mistakes in counting zeros. Using ratio_divide reduces the fraction 1000000000l/CLOCKS_PER_SEC to lowest terms, no matter the value of CLOCKS_PER_SEC. You can use rep as a shortcut for duration::rep, assuming this is inside of the clock's now() function. static_cast<rep> is a little safer than a C cast. I would expect the results to be the same, but it is good to code defensively. If you know a-priori that R::den is 1, you might: typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; static_assert(R::den == 1, "oops!"); return time_point( duration(static_cast<rep>(c)*R::num) ); Howard
Le 05/01/12 17:38, Howard Hinnant a écrit :
On Jan 5, 2012, at 10:30 AM, ivan.lelann@free.fr wrote:
Hi,
As of 1.48 Boost.Chrono contains code below for boost::chrono::process_real_cpu_clock::now() (boost\chrono\detail\inlined\win\process_cpu_clocks.hpp)
clock_t c = ::clock(); /* ... */ return time_point( duration(c*(1000000000l/CLOCKS_PER_SEC)) );
duration::rep is int64/nanoseconds, clock_t is long. This is VS2008, Win XP 32bits.
I think "c" should be cast to duration::rep before being multiplied. I randomly get negative time_point and this seems to be fixed by replacing "c" with "((duration::rep) (c))" in duration constructor :
duration( ((duration::rep) (c)) *(1000000000l/CLOCKS_PER_SEC))
Can someone confirm my view and that the fix is correct ? Your fix looks correct to me. I'm not positive what value CLOCKS_PER_SEC has on your platform. But here is a reformulation of your solution that might be a little safer (depending on what CLOCKS_PER_SEC actually is):
typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; return time_point( duration(static_cast<rep>(c)*R::num/R::den) );
Using giga avoids mistakes in counting zeros.
Using ratio_divide reduces the fraction 1000000000l/CLOCKS_PER_SEC to lowest terms, no matter the value of CLOCKS_PER_SEC.
You can use rep as a shortcut for duration::rep, assuming this is inside of the clock's now() function.
static_cast<rep> is a little safer than a C cast. I would expect the results to be the same, but it is good to code defensively.
If you know a-priori that R::den is 1, you might:
typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; static_assert(R::den == 1, "oops!"); return time_point( duration(static_cast<rep>(c)*R::num) );
Hi, yes this seems much more safer. Ivan please, could you create a ticket for this issue? Best, Vicente
----- Mail original -----
De: "Vicente J. Botet Escriba" <vicente.botet@wanadoo.fr> À: boost@lists.boost.org Envoyé: Jeudi 5 Janvier 2012 17:53:12 Objet: Re: [boost] [chrono] Possible bug in boost::chrono::process_real_cpu_clock::now() under Windows 32bits
Le 05/01/12 17:38, Howard Hinnant a écrit :
On Jan 5, 2012, at 10:30 AM, ivan.lelann@free.fr wrote:
Hi,
As of 1.48 Boost.Chrono contains code below for boost::chrono::process_real_cpu_clock::now() (boost\chrono\detail\inlined\win\process_cpu_clocks.hpp)
clock_t c = ::clock(); /* ... */ return time_point( duration(c*(1000000000l/CLOCKS_PER_SEC)) );
duration::rep is int64/nanoseconds, clock_t is long. This is VS2008, Win XP 32bits.
I think "c" should be cast to duration::rep before being multiplied. I randomly get negative time_point and this seems to be fixed by replacing "c" with "((duration::rep) (c))" in duration constructor :
duration( ((duration::rep) (c)) *(1000000000l/CLOCKS_PER_SEC))
Can someone confirm my view and that the fix is correct ? Your fix looks correct to me. I'm not positive what value CLOCKS_PER_SEC has on your platform. But here is a reformulation of your solution that might be a little safer (depending on what CLOCKS_PER_SEC actually is):
typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; return time_point( duration(static_cast<rep>(c)*R::num/R::den) );
Using giga avoids mistakes in counting zeros.
Using ratio_divide reduces the fraction 1000000000l/CLOCKS_PER_SEC to lowest terms, no matter the value of CLOCKS_PER_SEC.
You can use rep as a shortcut for duration::rep, assuming this is inside of the clock's now() function.
static_cast<rep> is a little safer than a C cast. I would expect the results to be the same, but it is good to code defensively.
If you know a-priori that R::den is 1, you might:
typedef ratio_divide<giga, ratio<CLOCKS_PER_SEC>>::type R; static_assert(R::den == 1, "oops!"); return time_point( duration(static_cast<rep>(c)*R::num) );
Hi,
yes this seems much more safer.
Ivan please, could you create a ticket for this issue?
Done : https://svn.boost.org/trac/boost/ticket/6361 Regards, Ivan
participants (3)
-
Howard Hinnant -
ivan.lelann＠free.fr -
Vicente J. Botet Escriba