[boost] Re: xtime_get unsafe on Windows for IA64

From: Michael Glassford (glassfordm_at_[hidden])
Date: 2004-08-27 14:29:36

Ben Hutchings wrote:
> Michael Glassford <glassfordm_at_[hidden]> wrote:
> 
>>Ben Hutchings wrote:
>>
>>>The implementation of the boost::xtime_get() function in
>>>libs/thread/src/xtime.cpp includes the following code for Windows:
> 
> <snip>
> 
>>>The pointer casts are not safe because although the representations
>>>of FILETIME and __int64 are compatible, FILETIME is only 32-bit-
>>>aligned whereas __int64 must be 64-bit-aligned on IA64 (in some
>>>processor modes).
>>>
>>>One can instead use union { FILETIME ft; __int64 i64; } and read out
>>>i64, though I'm not sure this code should be unconditionally using
>>>__int64 anyway.  There is a longer explanation at
>>><http://weblogs.asp.net/oldnewthing/archive/2004/08/25/220195.aspx>.
>>
>>How about this instead:
>>
>>     const boost::uint64_t TIMESPEC_TO_FILETIME_OFFSET =
>>         (static_cast<boost::uint64_t>(27111902UL) << 32)
>>         + 3577643008UL;
> 
> 
> I think this should use the UINT64_C macro instead of messing about
> with casts.

OK.

I see this is defined in boost/cstdint.hpp, but only if 
__STDC_CONSTANT_MACROS is #defined. How does this happen? Do I #define 
it myself, or does it get #defined some other way?

>>     const boost::uint64_t ft64 =
>>         (static_cast<boost::uint64_t>(ft.dwHighDateTime) << 32)
>>         + ft.dwLowDateTime;
>>
>>     xtp->sec = static_cast<int>(
>>         (ft64 - TIMESPEC_TO_FILETIME_OFFSET) / 10000000
>>     );
> 
> 
> The cast should be to int_fast64_t rather than to int.

OK. I added a typedef, xtime::xtime_sec_t, and used that instead.

>>     xtp->nsec = static_cast<int>((
>>             ft64 - TIMESPEC_TO_FILETIME_OFFSET
>>             - (static_cast<boost::uint64_t>(xtp->sec) * 10000000)
>>         ) * 100
>>     );
>>
>>?
> 
> 
> The cast should be to int_fast32_t.  Also the use of multiplication
> and subtraction to calculate the remainder seems to be a premature
> optimisation that obscures what's really going on.  

I agree.

OK. I also added a typedef, xtime::xtime_nsec_t, and used that instead.

 > A smart compiler
> can combine the division and remainder operations, and I have
> confirmed that Visual C++ 7.1 does so when generating optimised i386
> code (even for 64-bit division where it uses a subroutine rather than
> a single instruction).  So I would suggest:
> 
>      static const boost::uint64_t TIMESPEC_TO_FILETIME_OFFSET =
>         UINT64_C(116444736000000000);
> 
>      const boost::uint64_t ft64 =
>          (static_cast<boost::uint64_t>(ft.dwHighDateTime) << 32)
>          + ft.dwLowDateTime;
>      // or use the union trick which MS appears to have blessed
> 
>      xtp->sec = static_cast<int_fast64_t>(
>          (ft64 - TIMESPEC_TO_FILETIME_OFFSET) / 10000000
>      );
>      xtp->nsec = static_cast<int_fast32_t>(
>          ((ft64 - TIMESPEC_TO_FILETIME_OFFSET) % 10000000) * 100
>      );
> 
> Ben.

Mike