RE: [boost] Re: xtime_get unsafe on Windows for IA64
Michael Glassford <glassfordm@hotmail.com> 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.
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.
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. 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.
Ben Hutchings wrote:
Michael Glassford <glassfordm@hotmail.com> 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
Michael Glassford wrote:
Ben Hutchings wrote:
Michael Glassford <glassfordm@hotmail.com> 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?
Any help on how to use the Boost UINT64_C macro? Mike
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?
Any help on how to use the Boost UINT64_C macro?
Define __STDC_CONSTANT_MACROS at the start of the source *before* any std lib headers are included, then include <boost/cstdint.hpp>. One potential gotcha: note that the implementation of <boost/cstdint.hpp> may delegate to the platform's stdint.h , so we're relying on that not being buggy. Then: boost::int64_t i = UINT64_C(0x1234567812345678); John.
participants (3)
-
Ben Hutchings -
John Maddock -
Michael Glassford