# [Patch available] Several Currency defects on trunk since 0adf046ea0 ## Summary Commit 0adf046ea0 "Overhaul and bug fixes on how Currency types are propagated" made `nf_is_currency` load-bearing for correctness: `taddnode.do_currency_corrections` now assumes that a currency-typed node without that flag holds an unscaled value. Several places produce or substitute currency-typed nodes without maintaining the flag, and the corrections then apply the 10000 scalar where they should not, or omit it where they should. Four defects, all reachable from plain source, none of them diagnosed by a compiler message: | | needs | example | got | expected | |---|---|---|---|---| | 1 | inlining | `Inl * 16` | 1600000.0000 | 160.0000 | | 2 | nothing | `-c + 1` | -99999.0000 | -9.0000 | | 3 | nothing | `a div b`, `(a mod b) + 1` | 0.0003, 10001.0000 | 3.0000, 2.0000 | | 4 | `-Oofastmath` | `c / 2` | 0.0005 | 5.0000 | I have patches for 1 and 2, verified as described below. For 3 and 4 I have the diagnosis but no patch, because in both cases the right condition depends on design intent I would only be guessing at. They are in this report because they are the same regression and because defect 3 interacts with my patch for defect 1 (see "Interaction" below). That commit has an author date of 2024-09-12, but its committer date on trunk is 2026-08-15, four days before the 416b51be87 I am building from. So this is a recent trunk regression and no release is affected. Environment: native FPC 3.3.1 trunk 416b51be87, aarch64-win64, Windows 11 ARM64 on a Snapdragon X Elite. FPC 3.2.2 on aarch64-linux prints the expected values for every case below. That is supporting context rather than a clean bisect, since compiler version, target and OS all differ between the two. ## Defect 1: inlining loses the Currency scaling state Reproducer `currinline.pas`, attached. Every check compares an inlined construct against a non-inlined one with an identical body, so the non-inlined form is the oracle. ```pascal procedure Fill(out res: currency); begin PInt64(@res)^ := raw; { raw = 100000, i.e. currency 10.0000 } end; function Inl: currency; inline; { NoInl: same, not inlined } begin Fill(result); end; function AddInl(const c: currency): currency; inline; { AddNoInl: same, not inlined } begin result := c + 1; end; function LocInl(const x: currency): currency; inline; { LocNoInl: same, not inlined } var t: currency; begin t := x + 1; result := t * 2; end; ``` `fpc -O2 -Twin64 -Paarch64 currinline.pas`: ``` FAIL f * 16: inlined 1600000.0000 non-inlined 160.0000 FAIL 16 * f: inlined 1600000.0000 non-inlined 160.0000 FAIL f + 1: inlined 100001.0000 non-inlined 11.0000 FAIL f - 1: inlined 99999.0000 non-inlined 9.0000 FAIL f = 10: inlined FALSE non-inlined TRUE FAIL f < 11: inlined FALSE non-inlined TRUE FAIL param(var): inlined 100001.0000 non-inlined 11.0000 FAIL param(call): inlined 100001.0000 non-inlined 11.0000 FAIL local: inlined 2000020000.0000 non-inlined 22.0000 FAIL local - 1: inlined 20000199999999.0000 non-inlined 21.0000 ``` Three substitution sites are involved: - `doinline` (`optcall.pas:198`-`220`) replaces the call node with the inlined block. A call node returning `Currency` carries `nf_is_currency` (`ncal.pas:4706`, added by 0adf046ea0); the block does not inherit it. - `replaceparaload` replaces parameter loads with temp references. - `createlocaltemps` turns the inlined routine's locals into temps. For the latter two: `tloadnode.pass_typecheck` sets `nf_is_currency` for a currency variable (`nld.pas:491`), but `ttemprefnode.pass_typecheck` (`nbas.pas:1699`) does not, so a temp standing in for such a variable loses the flag. Not optimization dependent, it needs only inlining: 10 failures at `-O2`, `-O1` and `-O-`, OK with `{$INLINE OFF}`. Note that `-Si-` does not disable inlining in Delphi mode. Two things that look unaffected but are not evidence of anything. `f > 9` passes, but only because an operand inflated by 10000 satisfies it trivially, and `f < 11` fails. `f * f`, `f + f` and an inlined function returning another inlined currency call also pass; I have not established why, so I would not read that as those shapes being safe. `Currency` is what is special here: the same shapes with `Int64` or `Double` are correct. ### Patch ```diff --- a/compiler/optcall.pas +++ b/compiler/optcall.pas @@ -220,6 +220,15 @@ _n:=n; end; + { the inlined block takes the place of the call node, so it has to + inherit its currency scaling state: without it, the enclosing + taddnode sees an operand that is no longer marked as currency and + taddnode.do_currency_corrections applies the 10000 scalar a second + time } + if (cnf_return_value_used in callnode.callnodeflags) and + (nf_is_currency in callnode.flags) then + include(_n.flags,nf_is_currency); + PBoolean(arg)^:=true; {$ifdef EXTDEBUG_INLINE} --- a/compiler/nbas.pas +++ b/compiler/nbas.pas @@ -1703,6 +1703,11 @@ result := nil; resultdef := tempinfo^.typedef; + { a temp that stores a currency value holds it already scaled by + 10000, the same as a currency variable does + (see tloadnode.pass_typecheck) } + if is_currency(resultdef) then + Include(flags, nf_is_currency); end; ``` The `nbas.pas` hunk is the one to look at critically. It makes `ttemprefnode` behave like `tloadnode`, which sets the same flag two files away for the same reason, but it does so for every currency-typed temp reference in the compiler, not only the ones inlining creates. Two caveats I am aware of: - Reference temps (`ttempcreatenode.create_reference`) install the referenced expression as `tempinitcode` without an assignment that would normalise it, so for those the flag is an assertion about the aliased node rather than about a stored value. I could not construct a case where this misfires on this target, but the invariant is weaker there than the comment suggests. - It makes the flag more load-bearing: with a flagged temp on the left, `tassignmentnode` (`nld.pas:897`) will scale an unflagged currency-typed right-hand side on the way in. That is correct as long as there are no remaining producers of scaled-but-unflagged values. Defect 3 is exactly such a producer, which is why it is in this report. A narrower alternative would be to mark only the temps created for inlined parameters and locals, or to give `ptempinfo` an explicit scaling state. A third option is to make `do_currency_corrections` idempotent so that re-running it after inlining is harmless. I went with the flag propagation because it is the smallest change that restores the invariant the corrections already rely on, but which layer you want this fixed at is your call. Removing the two `Include(flags,nf_is_currency)` statements that 0adf046ea0 added to `ncal.pas` also makes `f * 16` correct, but it breaks `f + 1`, `f - 1`, `f / 2`, `f = 10` and the conversion to `Double` for inlined and non-inlined calls alike. Those statements are needed. ### Which pass misfires The caller is typechecked and first-passed before inlining happens (`psub.pas:2018`, then `do_optinline`). The first currency correction runs in `taddnode.pass_typecheck` (`nadd.pas:3561`). `doinline` then replaces the call and the tree is simplified again (`optcall.pas:253`), reaching `taddnode.simplify` and its second call to `do_currency_corrections` (`nadd.pas:741`). With the flags preserved that second run is a no-op. ## Defect 2: unary minus loses the Currency scaling state Reproducer `currneg.pas`, attached. No inlining, no optimization, nothing unusual: ```pascal var c: currency; d: double; begin c := 10; writeln(-c); { -10.0000, correct } writeln(-c + 1); { -99999.0000, expected -9.0000 } d := -c; writeln(d); { -100000.0000, expected -10.0000 } writeln(-c = -10); { FALSE, expected TRUE } end. ``` The program reports 5 failures at `-O2`, and the same at `-O1` and `-O-`. | expression | result | expected | |---|---|---| | `-c` | -10.0000 | -10.0000 | | `-c * 2` | -20.0000 | -20.0000 | | `0 - c + 1` | -9.0000 | -9.0000 | | `-c + 1` | -99999.0000 | -9.0000 | | `1 + -c` | -99999.0000 | -9.0000 | | `-c - 1` | -100001.0000 | -11.0000 | | `-c` to a `double` | -100000.0000 | -10.0000 | | `-c = -10` | false | true | The negation on its own is right, and spelling it out as `0 - c + 1` is right. The negation as an operand of an addition, a subtraction or a float conversion is off by a factor of 10000; as an operand of a comparison it yields the wrong boolean. `tunaryminusnode.pass_typecheck` (`nmat.pas:1141`) has an empty currency branch: ```pascal resultdef:=left.resultdef; if is_currency(left.resultdef) then begin end else if left.resultdef.typ=floatdef then ``` It appears intended to keep currency out of the excess-precision handling below it, and dates from 272a0e3e26 (2021), where it was harmless. It became a defect when 0adf046ea0 gave the flag its present meaning. That commit does touch `nmat.pas`, but only the `divn` path. ### Patch ```diff --- a/compiler/nmat.pas +++ b/compiler/nmat.pas @@ -1141,6 +1141,10 @@ resultdef:=left.resultdef; if is_currency(left.resultdef) then begin + { negating a currency value does not change its scaling, so the + result is scaled exactly like the operand } + if nf_is_currency in left.flags then + Include(flags,nf_is_currency); end else if left.resultdef.typ=floatdef then ``` On targets where `s64currencytype` is a `floatdef`, `tunaryminusnode.simplify` additionally re-associates through `is_real(left.resultdef)` (`nmat.pas:1045`-`1090`) and rebuilds `muln`/`slashn` nodes without copying flags. That path is unreachable on `orddef` targets, so this patch may be incomplete for i386 and non-Windows x86_64. ## Defect 3: div and mod on two Currency variables Reproducer `currdivmod.pas`, attached. No inlining, no optimization. The two operators fail in opposite ways, which is worth separating. `div` stores the wrong value. The raw quotient is not rescaled: | | trunk | 3.2.2 | |---|---|---| | `PInt64(@r)^` after `r := a div b` | 3 | 30000 | | `writeln(a div b)` | 0.0003 | 3.0000 | | `(a div b) = 3` | true | true | | `(a div b) + 1` | 4.0000 | 4.0000 | `mod` stores the right value but it is not marked as scaled, so the surrounding expression scales it again: | | trunk | 3.2.2 | |---|---|---| | `PInt64(@r)^` after `r := a mod b` | 10000 | 10000 | | `writeln(a mod b)` | 1.0000 | 1.0000 | | `(a mod b) = 1` | false | true | | `(a mod b) + 1` | 10001.0000 | 2.0000 | So `a div b` is self-consistent as a raw integer and only wrong once converted, while `a mod b` is stored correctly and only wrong once used. 0adf046ea0 tightened the rescaling condition in `tmoddivnode.pass_typecheck` (`nmat.pas:454`): ```diff if (nodetype=divn) and + not(nf_internal in flags) and not(nf_is_currency in flags) and - is_currency(resultdef) then + is_currency(resultdef) and + (nf_is_currency in left.flags) and + not (nf_is_currency in right.flags) then ``` With two currency variables both operands are flagged, so `not (nf_is_currency in right.flags)` is false and the `* 10000` that turns the raw Int64 quotient back into a Currency value is never inserted. That accounts for the `div` half exactly. The `mod` half is the flag not being set on the result node, so a correctly scaled value is taken for an unscaled one. I have not proposed a patch: the right condition depends on what each operand flag state is meant to encode here, and I would be guessing. ## Defect 4: fastmath drops the scaling of a Currency division Reproducer `currfastmath.pas`, attached: ``` -O2 OK -O2 -Oofastmath FAIL c / 2: got 0.0005, expected 5.0000 -O4 FAIL c / 2: got 0.0005, expected 5.0000 -O4 -OoNOFASTMATH OK ``` The reciprocal transform in `taddnode` (`nadd.pas:1353`-`1396`) builds `crealconstnode.create(1/value, resultdef)` with a currency `resultdef`, unscaled and unflagged. Its guard against touching currency inspects the right operand's def and does not account for a tree the currency corrections have already rewritten. No patch proposed, same reason as defect 3. ## Interaction between defect 3 and the patch for defect 1 Worth knowing before diffing test output. With `a = 10`, `b = 3` and `function Id(const x: currency): currency; inline; begin result := x; end;`: | | unpatched | patched | 3.2.2 | |---|---|---|---| | `Id(a mod b) + 1` | 10001.0000 | 2.0000 | 2.0000 | | `Id(a div b) + 1` | 4.0000 | 1.0003 | 4.0000 | The `mod` row is fixed by the patch. The `div` row changes from a value that matches 3.2.2 to one that does not: the patch makes the inlined form agree with the un-inlined `a div b`, which is itself wrong because of defect 3. So this is the patch propagating defect 3 consistently rather than a new defect, but it does mean the patch for defect 1 should land together with a fix for defect 3, not before it. Similarly, `-c / 2` at `-O4` goes from -50000.0000 to -0.0005 with the defect 2 patch; both are wrong, and the second is consistent with what `c / 2` already does under fastmath (defect 4). ## Tests Four testsuite programs are attached, in the `webtbs` format, named `twNNNNN*.pp` so the number can be filled in: | file | covers | on trunk | with the patches | |---|---|---|---| | `twNNNNN.pp` | defect 1, eleven shapes including parameter and local | exit 1 | exit 0 | | `twNNNNNb.pp` | defect 2 | exit 1 | exit 0 | | `twNNNNNc.pp` | defect 3, marked `%KNOWNRUNERROR=1` | exit 1 | exit 1 | | `twNNNNNd.pp` | defect 4, `%OPT=-O2 -Oofastmath`, marked `%KNOWNRUNERROR=1` | exit 1 | exit 1 | The two unfixed defects are marked as known errors so they document the behaviour without turning the suite red; the markers should come off when they are fixed. `twNNNNN.pp` compares each inlined construct against a non-inlined one with an identical body rather than against a literal expected value, so it stays valid regardless of how the scaling is represented internally. `twNNNNNc.pp` checks `PInt64(@r)^` directly for the `div` case, because `(a div b) = 3` is true on trunk even though the stored value is wrong. ## Why the testsuite does not catch this Of the ten currency tests in the suite (`tbs/tb0683`, `test/cg/taddcurr`, `webtbs/tw17904`, `tw18704`, `tw19077`, `tw22561`, `tw28748`, `tw28749`, `tw36013`, `tw40928`), nine pass on the unpatched trunk compiler. None of them negates a currency variable inside an expression (`taddcurr` only negates constants), none applies `div` or `mod` to two currency variables, and none calls an inlined function returning currency. `tw40928` fails, with and without any of my changes. ## Verification On 416b51be87 plus the two patches, aarch64-win64: - `currinline.pas` and `currneg.pas` at `-O2`: OK. `currdivmod.pas` and `currfastmath.pas` are unchanged by the patches, as expected. - mORMot2's `TDecimal128` test, which is where this started: 640 failures out of 17446 assertions before, 0 after. It calls `TDecimal128.ToCurr`, an inline method returning `Currency`, as `v.ToCurr * 16 = i`. - the whole mORMot2 suite: 835 failures out of 164,301,386 assertions before, 167 out of 164,402,287 after. Per suite: Core base 26 to 4, Core process 640 to 0, Core compression 3 to 3, SQLite 4 to 4, SOA 162 to 156. The remaining failures appear unrelated to currency (Windows API and SOA tests), though I have not diagnosed each one. - bootstrap is stable: compiling the patched compiler with itself twice gives bit-identical binaries (sha256 5F47C6E8411F90E70A3D0D683499E72D08F4838A2063C86A106A088407EE38C4). That binary carries both patches. - the full FPC testsuite, `make allexectests CPU_TARGET=aarch64 OS_TARGET=win64`, run with and without the patches: 6783 tests executed in both, identical sets of executed and failing tests. The baseline failure rate on this machine is high (3239 `ExecuteRedir ... Failed to execute` in both runs) and looks like a suite setup problem on this fresh aarch64-win64 installation, so this shows no regression rather than a green suite. - #40941 (currency constant multiplication scaled by 10000) stays fixed: `Currency(10000) * Currency(10000)` gives `100000000.0000` before and after. ## Scope None of the affected code is target specific. I could only test aarch64-win64, since this machine has no second target compiler. Per `psystem.pas:326`-`334`, x86_64-win64 uses the same `orddef` currency representation as aarch64, so these defects are expected to reproduce there; i386 and non-Windows x86_64 use the `floatdef` representation, where the corrections take different branches, so the symptoms may differ. I found no existing issue covering these. The closest are #40941 (currency constant multiplication, closed) and #8101 (unary minus on currency, closed in 2.2.0, a precision problem in the negation itself rather than a scaling-state problem in the enclosing expression). [fpc-currency-fix.patch](/uploads/c923f2840856da49b9bcbbd47a07b336/fpc-currency-fix.patch) [fpc-currency-repro.zip](/uploads/3419990f89a3c8dcc2bb62cf9c332e23/fpc-currency-repro.zip) [fpc-currency-tests.zip](/uploads/29b3fadbc62ca9eb55cb274e45564df0/fpc-currency-tests.zip)