Reduce Datetime.now's nanoseconds without narrowing through a C long - #26
Conversation
`now` derived its nanoseconds field with
(Long.to-int (mod (Uint64.to-long t) 1000000000l))
where `t` is the `Uint64` from `System.nanotime`. `Uint64.to-long` is
`return (long)x;` in carp_stdint.h, and C `long` is 32 bits on every ILP32
target (32-bit Linux, armhf) and on Windows x64, so the modulo ran on the
sign-extended low 32 bits of a 64-bit count.
Measured on a 32-bit armhf host, where sizeof(long) is 4:
System.nanotime = 1787138980662882507
Uint64.to-long = 715796683 ; == nanotime mod 2^32
Datetime.now ns = 715797220
correct answer = 662882507
The field is a function of the truncated low 32 bits, so it wraps
discontinuously about every 4.29 s of wall clock — and once that low word's
top bit is set the cast sign-extends and C's `%` returns a negative result,
so the field can come back negative outright. A sample taken while verifying
this reported -272749194. Every runner in the org's CI is 64-bit, which is
why it survived.
The reduction now stays in `Uint64` throughout — written as
`(- t (* (/ t n) n))`, since core registers no `Uint64.mod` — and only the
result, which is below one billion, is narrowed. It lives in
`Datetime.subsecond-nanos` so it can be pinned by assertions everywhere, not
only on the targets where the truncation is observable.
Two of the six vectors have teeth on 64-bit hosts too: at 2^63 and at
`Uint64.MAX` the old `(long)` cast preserves the bits but yields a negative
`Long`, and the old expression then hands back a negative nanoseconds field.
A seventh assertion brackets `Datetime.now` between two `System.nanotime`
samples so the call site is pinned as well, standing down on the rare sample
pair that straddles a second boundary.
`Uint64.to-long` was the only place in time.carp where a genuinely 64-bit
value is narrowed; the remaining `Long` conversions all carry timezone
offsets, which never exceed a day.
There was a problem hiding this comment.
Build & Tests
Checked out 69c7290. The branch contains origin/master's head 3437730 (the 0.5.3 release), so nothing here is filed against a stale base.
carp -xwith CI's exact command — 310 passed, 0 failed, rc 0 read from the bare command, not through a pipe. Matches the body's count.- This host is the affected target:
sizeof(long) == 4,sizeof(void*) == 4. So unlike CI's two 64-bit runners, everything below is a direct observation rather than an argument about what an ILP32 compiler would do.
The bug and the fix, reproduced
I rebuilt the pre-PR expression alongside the new helper and ran both over every vector in your table. It reproduces exactly, row for row:
uint64 old new
999999999 999999999 999999999 999999999
1.5e9 1500000000 500000000 500000000
2^32 4294967296 0 294967296
1787138980662882507 1787138980662882507 715796683 662882507
2^63 9223372036854775808 0 854775808
Uint64.MAX 18446744073709551615 -1 709551615
The mechanism is as described: Long is int64_t (core.h:22), but Uint64_to_MINUS_long is return (long)x; (carp_stdint.h:267), so on ILP32 it truncates to the low 32 bits and sign-extends them back into an int64_t. Uint64.+ - * / are all native uint64_t operations, so (- t (* (/ t billion) billion)) is an exact unsigned modulo, and the one remaining Uint64.to-long narrows a value already below 1e9.
The negative-field claim is real, not just a Uint64.MAX artifact. On a realistic wall-clock count whose low word has its top bit set:
count = 1787138983123456789
old-expr = -118596331
subsecond = 123456789
and a live sample taken during the run would have reported -775322691 where the answer was 302213565. Post-fix, Datetime.now's field agrees with nanotime mod 1e9 on this host.
Worth recording because it makes the field meaningful rather than merely non-negative: System_nanotime reads CLOCK_REALTIME (carp_system.h:22-25), so nanotime mod 1e9 is exactly tv.tv_nsec — the true sub-second part of wall time. Had it been a monotonic clock this would have been a tidier way to compute a meaningless number.
Do the new assertions have teeth?
Two mutants, restoring from a backup between them:
- Helper reverted to the truncating expression →
rc 4, 306 passed, and the four failures are precisely the four vectors your table predicted would discriminate:2^32, the realistic reading,2^63,Uint64.MAX. The other two rows are honestly documented as non-discriminating and they are. - Helper left correct, only the call site in
nowreverted to the old inline expression →rc 1, and"now reads its nanoseconds off the full-width clock"fails.
That second one is the one I care about. The usual failure mode for a change shaped like this is a well-tested new helper wired in by an untested line, so that reverting the wiring alone stays green. It does not stay green here. The seventh assertion is doing real work.
Findings
1. The Windows x64 claim in the description does not hold — nanotime is stubbed there.
The body says the field is wrong "on every ILP32 target ... and on Windows x64", and the LLP64 reasoning about long is correct in the abstract. But under _WIN32, carp core defines
uint64_t System_nanotime() {
return 0;
}(carp_system.h, the #ifdef _WIN32 branch). The input is always 0, so 0 mod 1e9 is 0 before this change and 0 after it — nothing truncates because there is nothing to truncate. On Windows the nanoseconds field is not wrong-by-truncation, it is absent, and this PR neither fixes nor worsens that.
No code change wanted. It is worth correcting in the description because it overstates the reach, and worth knowing separately: Datetime.now has no sub-second resolution at all on Windows, which is a real gap this PR incidentally documents but does not close.
2. Cosmetic: the "Also checked" line numbers are pre-fix. The remaining Long conversions are cited as 568–569 and 1045–1046; on the branch they are at 577–578 and 1054–1055 — the new binder shifts everything below it by nine lines. The substance is right: after this change to-long appears exactly once in time.carp (:469, the safe one), and every other Long conversion carries a Timezone.delta, bounded by a day.
Checked and clean
- The
nanostest helper is not defensive over-engineering, it is required. My own probe hit exactly the trap it avoids: I masked with a literal4294967295l, which folded to-1on this host, soUint64.from-longgave me all-ones and the mask silently became a no-op. Assembling vectors from parts below 2^31 is the only way these constants mean the same thing on both word sizes. - The seventh assertion's stand-down guard is sound.
(> (- b a) billion)catches a second boundary; ifSystem.nanotimeever went backwards theUint64subtraction wraps to a huge value and also trips it, so a backwards step cannot produce a false failure through that path. The one residual flake I can construct is aCLOCK_REALTIMEstep landing between the two samples and moving the clock backwards by less than a second — sub-microsecond window, not worth guarding. - No changelog in this repo, so nothing to update;
docs/Datetime.htmlcarries the new binder, consistent with how previous PRs here have handled a new public function. - Making
subsecond-nanospublic rather thanprivateis the right trade: it is the only way the reduction is pinnable on the 64-bit runners where the truncation is invisible, and the four discriminating vectors above show that pinning is not theoretical.
Verdict: merge
The bug is real and reproduces exactly as described on this ILP32 host, the fix is exact over the whole Uint64 range, and the tests kill both the helper mutant and the call-site mutant. Finding 1 is a correction to the PR description's platform claim, not to the code.
Datetime.nowreports a wrong nanoseconds field on every ILP32 target (32-bitLinux, armhf) and on Windows x64. The field came from
where
tis theUint64returned bySystem.nanotime.Uint64.to-longisreturn (long)x;incarp_stdint.h, and Clongis 32 bits on those targets,so the modulo ran on the sign-extended low 32 bits of a 64-bit count.
Measured on a 32-bit armhf host (
sizeof(long) == 4)It is not merely inaccurate. The field is a function of the truncated low 32
bits, so it wraps discontinuously about every 4.29 s of wall clock; and once
that low word's top bit is set, the cast sign-extends and C's
%returns anegative result, so the field can come back negative outright — a sample taken
while verifying this reported
-272749194. Both CI runners (macos-latest,ubuntu-latest) are 64-bit, which is why this survived.The fix
The reduction now stays in
Uint64throughout, written as(- t (* (/ t n) n))because core registers no
Uint64.mod. Only the result is narrowed, and it isbelow one billion, so it always fits an
Int. Nothing 64-bit wide passesthrough
Uint64.to-longany more.It is extracted as
Datetime.subsecond-nanosso the reduction can be pinned byassertions that run on every target, not only where the truncation is
observable. That is the one bit of new public surface here; the alternative was
to leave the arithmetic untestable outside a 32-bit host.
Tests
Seven new assertions, all passing (
310 passed, 0 failed). Six pinsubsecond-nanoson fixed vectors; the counts are assembled from parts under2^31, since a wider
Longliteral itself truncates on a 32-bit host and wouldmake the vectors target-dependent.
Checked what each vector does against the old expression on this armhf host
(want / old):
9999999991.5e92^3217871389806628825072^63Uint64.MAXThe
2^63andUint64.MAXrows also have teeth on a 64-bit host: there the(long)cast preserves the bits but yields a negativeLong, and the oldexpression returns
-854775808and-1respectively. So CI exercises the fixrather than just carrying the assertions along.
The seventh assertion brackets
Datetime.nowbetween twoSystem.nanotimesamples, so the call site is pinned and not only the helper. It stands down on
the rare sample pair that straddles a second boundary (guarded on both
b - a < 1e9andlo <= hi, which together rule out a wrap). Confirmed itfails against the unfixed
nowand passes against the fixed one.Also checked
Uint64.to-longattime.carp:474was the only place in the file where agenuinely 64-bit value was narrowed. The remaining
Longconversions(
time.carplines 386, 426, 443, 568–569, 1045–1046) all carry timezoneoffsets from
tm_gmtoff, which never exceed a day and so cannot truncate.No changelog: this repo has none.
docs/Datetime.htmlis regenerated for thenew binder, matching what previous PRs here have done.
Verified locally with CI's exact test command
(
(add-cflag "-Werror")+(load "test/time.carp")), pluscarp-fmt --checkand
angler, all green.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.