Fix M*: use __int128 for a genuine 128-bit double-cell product (FABRIC-3.md §XII.4)
mixed_math_word_m_star() confused "double" (two full cell_t-width cells,
128 bits total on this 64-bit build -- what D+/D-/D./etc. all actually
expect) with "the low/high 32-bit halves of a single 64-bit product" --
code clearly written assuming cell_t is 32-bit. It computed an ordinary
64-bit `long long` product (already wrong for any true product exceeding
64 bits, since long long is the same width as one cell here) and split
that into 32-bit halves via `result & 0xFFFFFFFF` / `result >> 32`. For a
small negative product like -56088, this produced a positive, zero-
extended low cell paired with a correctly-looking dhigh=-1 -- D.'s
overflow check (correctly) rejected the resulting malformed double, on
every architecture, every time (this bug was never architecture-specific,
unlike the D+/D-/DNEGATE/d_compare family already fixed in bea8d74/
1a716c8).
Fixed via __int128 for a genuine 64x64->128-bit multiply, no truncation --
an already-established safe pattern in this codebase for exactly this
operation (src/starkernel/crypto/fe25519.c/scalar25519.c already use it;
src/starkernel/arch/amd64/timer.c's own doc comment confirms __int128
multiply/shift-by-constant compile cleanly with zero undefined symbols on
all three target toolchains -- only division needs unavailable libgcc
support, not used here).
Verified: rebuilt and booted all three architectures clean. T13/T14 both
correct everywhere (56088/-56088). Manual cases confirm genuine 128-bit
precision: -1 -1 M* -> 1; 1000000000000 1000000000000 M* -> dhigh=54210
dlow=2003764205206896640 (10^12 x 10^12 = 10^24, correctly exceeding 64
bits). Full 24-case exerciser reran clean on all three, no regressions.
Found while verifying, NOT fixed (report only, out of scope of this
request): mixed_math_word_m_slash_mod() (M/MOD, the very next function in
the same file) has the identical bug class -- confirmed genuinely broken
for a true wide double (feeding this fix's own correct large-magnitude M*
output into M/MOD produces a result wrong by many orders of magnitude and
the wrong sign). Flagged in FABRIC-3.md for a future fix request.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EXieurDfDSsDFdnSyusuWo
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
1a716c8048
commit
9a09949c69
@@ -121,9 +121,26 @@ void mixed_math_word_m_minus(VM *vm) {
|
||||
* @brief M* ( n1 n2 -- d )
|
||||
*
|
||||
* Multiplies two single-cell signed integers, producing a double-cell result.
|
||||
* On 64-bit builds the product is computed via @c long long and the low 32 bits
|
||||
* go to the deeper stack cell while the high 32 bits go to TOS. On 32-bit builds
|
||||
* the result fits in a single cell so the high cell is pushed as 0.
|
||||
* A "double" here is two full @c cell_t-width cells (128 bits total on a
|
||||
* 64-bit build), matching what D+/D-/D./etc. all expect -- NOT the low/high
|
||||
* 32-bit halves of a single 64-bit product. The previous implementation
|
||||
* confused the two: it computed an ordinary 64-bit @c long long product
|
||||
* (already truncated/overflowed for any operand pair whose true product
|
||||
* doesn't fit in 64 bits) and then split THAT into 32-bit low/high halves,
|
||||
* as if @c cell_t were 32-bit. For a small negative product like
|
||||
* `-123 456 M*` (-56088, which fits entirely in one 64-bit cell with sign
|
||||
* extension), that produced a positive, zero-extended @c dlow paired with a
|
||||
* correctly-looking @c dhigh=-1 -- a malformed double that D.'s overflow
|
||||
* check (correctly) rejected as DOUBLE-OVERFLOW on every architecture,
|
||||
* every time a negative product was printed (FABRIC-3.md §XII.4 bug 1).
|
||||
*
|
||||
* Fixed via @c __int128 (a genuine 64x64->128-bit multiply, no truncation)
|
||||
* -- an established, safe pattern in this codebase for exactly this
|
||||
* operation: see src/starkernel/crypto/fe25519.c and scalar25519.c, and
|
||||
* src/starkernel/arch/amd64/timer.c's own doc comment confirming
|
||||
* __int128 multiply/shift-by-constant compile cleanly with zero undefined
|
||||
* symbols on all three target toolchains (only *division* needs libgcc
|
||||
* support unavailable in this freestanding build -- not used here).
|
||||
*
|
||||
* Stack effect: ( n1 n2 -- d_low d_high ) TOS = high
|
||||
*
|
||||
@@ -139,9 +156,9 @@ void mixed_math_word_m_star(VM *vm) {
|
||||
cell_t n1 = vm_pop(vm);
|
||||
|
||||
if (sizeof(cell_t) == 8) {
|
||||
long long result = (long long) n1 * (long long) n2;
|
||||
cell_t dlow = (cell_t)(result & 0xFFFFFFFFLL);
|
||||
cell_t dhigh = (cell_t)(result >> 32);
|
||||
__int128 result = (__int128) n1 * (__int128) n2;
|
||||
cell_t dlow = (cell_t)(unsigned __int128) result; // low 64 bits
|
||||
cell_t dhigh = (cell_t)(result >> 64); // high 64 bits, sign-preserving
|
||||
vm_push(vm, dlow); // lo first
|
||||
vm_push(vm, dhigh); // hi last (TOS)
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user