Skip to content

Commit

Permalink
REVIEW ME Avoid overflow from 64-bit A * B / C in `std::sys::window…
Browse files Browse the repository at this point in the history
…s::time`.

I should check if this method for "safer" multiplication by a ratio is
already provided somewhere in `core::num`.  (And if not, add it.)

Also, I should look into whether I have any better options here, and
whether I should just be calling the WrappingOps or OverflowingOps
anyway. But this should serve for the short term.
  • Loading branch information
pnkfelix committed Feb 25, 2015
1 parent a05148f commit b6d2ccb
Showing 1 changed file with 27 additions and 1 deletion.
28 changes: 27 additions & 1 deletion src/libstd/sys/windows/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,33 @@ impl SteadyTime {
}

pub fn ns(&self) -> u64 {
self.t as u64 * 1_000_000_000 / frequency() as u64
// Want to multiply self.t by the ratio `1_000_000_000 / frequency()`,
// but want to avoid overflow on the multiply.
// Solution: split self.t into separate high- and low-order parts:
// T = A * 2^32 + B
//
// (A * 2^32 + B) * G / F
// =
// A * G / F * 2^32 + B * G / F
// =
// (A * G div F + A * G rem F / F) * 2^32 + B * G / F
// =
// A * G div F * 2^32 + A * G rem F * 2^32 / F + B * G / F
// =
// A * G div F * 2^32 + (A * G rem F * 2^32 + B * G) / F
// ~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// hi lo

let f = frequency();
let g = 1_000_000_000;
let a = (self.t as u64) >> 32;
let b = (self.t as u64) & 0xFFFF_FFFF;
let ag = a * g;

let hi = ag / f << 32;
let lo = ((ag % f << 32) + b * g) / f;

hi + lo
}
}

Expand Down

1 comment on commit b6d2ccb

@pnkfelix
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, funny, the version of the code I made on its own ran, but then when I transcribed it into this file I obviously took a bad shortcut. (This commit does not build because of a type mismatch; fn frequency returns an i64 but we need a u64 here.)

Please sign in to comment.