Переглянути джерело

TP3-ARGORDER: push each argument as it is parsed, and read the frame from BP+4 backwards

A call read its whole argument list before pushing any of it, so a value that
existed only in AX (kind 2) did not survive the parse of the argument after it.
PSRC8 cproc/cprlp1/cprlp2 does the opposite: CALL exprsave then CALL epushax
for each argument AS IT IS READ, and only then the CALL.

Three parsers deferred the pushes - ParseCallArgs (a call as a statement),
ParseCall (a function inside an expression) and IoCall (write/writeln/read) -
and the callee was inverted in the other half: parameter offsets were handed
out in declaration order from BP+4, while an argument pushed first ends up
farthest from BP. A one-argument call cannot tell the two apart, which is all
Runtime.mod has, so nothing outside a fixture had ever seen it.

Fix, three sites plus one:
  * ParseCallArgs / ParseCall: LoadAtom + EmPushAx right after each ParseExpr;
    the deferred args[] loop is gone.
  * IoCall: the parse loop and the emit loop are one loop, so an argument is
    pushed (or its runtime call made) before the next argument is read. This
    also closes IoCall's second loss - it used to push an argument after the
    call made for an inline string literal in between.
  * ProcFunc: once the parameter list has been read, remap [nestMark, symTop)
    with `off := parmOff + 2 - off`, which sends 4 + 2*(k-1) to 4 + 2*(n-k):
    the last declared parameter lands at BP+4, where the first-pushed argument
    is. That range is exactly the parameters - the parameter's own NewSym is
    the only symbol created inside the loop.

New fixture t36_argclobber (eight hand-derived lines) covers all four. It was
compiled and run BEFORE this change, and both oracles came back red for the
same lines:

  want  5 3 7 / 0 8 / 8 0 / 8 0 14 / 12 0 / TRUE FALSE / 19 / FALSE 1
  got   5 3 7 / 0 8 / 0 0 / 0 0 14 / 0 0 / FALSE FALSE / 10 / TRUE 544

The two plain-name lines were already green and stay green; they are the rows
that would catch a fix flipping only one of the two halves, and they were seen
green before the fix rather than only after it.

No expectation row was re-baselined. t36's row was written while the compiler
was still broken and its numbers held, because the reorder emits the same
bytes in a different order. t28's row held because the remap hands the same
displacements to different names: its two disp16 reads are still +128/+142
(now p8/p1 where they were p63/p70), and its printed sum still comes from the
slots the sixteen pushed arguments land in (p55 + p70 = 1 + 16). The t28
source, its comment and check_framedisp's rule were updated to the TP3 frame
rule instead.

Gates: tests/run_all.sh rc=0, OVERALL: ALL PASS (all fifteen checks);
tests/nonvacuity.sh rc=0, "non-vacuity: 62 ok, 0 failed" with Compiler.mod
restored byte for byte. t36's in-suite mutation cases do not exist yet - its
proof is the pre-fix run above - and SUMMARY.md says so in both the Non-vacuity
section and Next steps rather than letting the 62 imply coverage it has not.
Eric Streit 2 днів тому
батько
коміт
edd057f5ce

+ 111 - 39
SUMMARY.md

@@ -26,6 +26,7 @@ manual, not guessed.
 | 8086-legal conditional branches and `SETcc` | `v-TP3-8086-LOWERING` | done, **the emitted code no longer contains an opcode the 8086 lacks** |
 | `CmdRun` (the `R` key) + `Exec86`, the in-process 8086 interpreter | `v-TP3-CMDRUN` | done, **a second execution oracle: 33/33 fixtures agree with qemu byte for byte, and `R` runs one inside the shell** |
 | Image layout in one file; a check's sweep region measured, not restated | `v-TP3-IMAGELAYOUT` | done, **every reader of a linked `.COM` imports `tests/comimage.py`, and the region `check_framedisp` sweeps is two readings of the file required to agree** |
+| Arguments pushed as they are parsed; the parameter frame read from BP+4 backwards | `v-TP3-ARGORDER` | done, **`t36_argclobber` runs: a computed argument survives the parse of the next one, and the callee's slots match the order they were pushed in** |
 
 Every row that names a tag has one, and every tag points at a commit on
 `master`; verified by diffing the rows against `git tag -l`, which is how
@@ -119,11 +120,11 @@ needing a squint. It was checked for vacuousness by reverting the string
 scanner fix: the suite went red with exit 1, and came back green only when the
 fix was restored.
 
-**33 of 36 fixtures compile clean**, up from 1 (the empty program) when the
+**34 of 37 fixtures compile clean**, up from 1 (the empty program) when the
 direct harness was first built.
 
 ```
-compile matrix: 36 passed, 0 failed (of 36)
+compile matrix: 37 passed, 0 failed (of 37)
 ```
 
 Compiling: `t01` minimal · `t04` var+assign+`writeln` · `t06` two args ·
@@ -138,16 +139,17 @@ supplied input file · `t30` a counted `for` whose bounds come from an
 expression · `t31` a value parameter *and* a call across statements ·
 `t32` `EXIT` out of a `for` body · `t33` all six comparisons · `t34` the
 operators nothing else uses (`div mod and or`, unary `-`, a variable `*`) ·
-`t35` `not`, both halves of TPSRC9's `neglevel` split.
+`t35` `not`, both halves of TPSRC9's `neglevel` split · `t36` a call whose
+arguments are computed in various positions, each one keeping its own value.
 
 Failing, all deliberately: `t14` `array [1..5] of integer` at its point of use
 and `t25` a string literal used as a *value* (`s := 'hi'`) both → `ENoLib`
 (102), the original's "not implemented" path; `uierror` is a deliberate syntax
 error (41) used by the UI test.
 
-`run_com_tests.sh` additionally links **all 33 that compile** to a real `.COM`
+`run_com_tests.sh` additionally links **all 34 that compile** to a real `.COM`
 and re-verifies the bytes with an independent Python checker that *measures*
-the layout instead of restating it: `independent .COM check: 33 checked, 0
+the layout instead of restating it: `independent .COM check: 34 checked, 0
 failed`. That last part was itself a bug fix — see "the two restated
 constants" below.
 
@@ -264,16 +266,16 @@ and `tests/check_comimage.py` asserting there is only one copy of each.
 ### Execution under qemu — `tests/run_com_exec.py`
 
 The check that cannot be written as a byte comparison, so also the one that
-finds the most: 33 fixtures are compiled to `.COM`, put on a floppy, booted,
+finds the most: 34 fixtures are compiled to `.COM`, put on a floppy, booted,
 and their serial output compared to a committed `.out` file **exactly** — CRLF
 included — plus the exit code passed to `INT 21h AH=4Ch`.
 
 ```
-execution: 33 passed, 0 failed (of 33)
+execution: 34 passed, 0 failed (of 34)
 ```
 
 This is no longer the only execution oracle: `run_exec86.py` below runs the
-same 33 images a second time, in a different machine, and requires the two to
+same 34 images a second time, in a different machine, and requires the two to
 agree byte for byte.
 
 It also found bug 33 — the branch polarity inverted in *every* conditional in
@@ -320,11 +322,11 @@ The only check that asks a question about the *target* rather than about the
 compiler: **does the 8086 have this instruction at all?**
 
 ```
-8086 check: 35 comparison sites, 31 lowered to a Boolean value, 13 lowered to a branch
-            value conditions  : = x6  <> x2  < x5  >= x3  <= x2  > x13
+8086 check: 38 comparison sites, 34 lowered to a Boolean value, 13 lowered to a branch
+            value conditions  : = x6  <> x2  < x5  >= x3  <= x2  > x16
             branch conditions : IF / REPEAT x9  CASE x2  FOR downto x1  FOR to x3
             runtime: 436 bytes, 219 swept, 0 0F-prefixed
-            program code: 30 of 33 fixtures swept end to end, 2897 bytes
+            program code: 31 of 34 fixtures swept end to end, 3373 bytes
             t33_cmpops: 13 comparisons matched against their source operators, in order
             clause H: 8 of 8 fixtures matched the branch conditions read off their source
 ```
@@ -378,7 +380,7 @@ check runs every fixture through it as well as through qemu and requires the
 two to agree **byte for byte**:
 
 ```
-exec86: 33 passed, 0 failed (of 33), cross-checked against qemu
+exec86: 34 passed, 0 failed (of 34), cross-checked against qemu
 ```
 
 Three assertions per fixture, in order of how much they are worth: the output
@@ -446,13 +448,25 @@ the `[BP+off]` rule, the behavioural bugs, the emitter-name audit of
 helper and the three checks that read it, the 8086 lowering, the interpreter
 itself, and the `R` key.
 
+One assertion is proved *outside* that suite, and the difference is stated
+rather than papered over: `t36_argclobber`'s `.out` was compiled and run
+against the compiler as it stood **before** the argument-order fix, on both
+oracles, and came back red with every line whose arguments included a computed
+value wrong while the two lines built from plain names stayed correct — so the
+two that would catch a one-sided flip were seen green before the fix, not after
+it. The evidence is quoted in the commit that introduces the fixture, and green
+followed only with the fix in. What is *not* there yet is the in-suite form of
+that proof: reverting each of the three call parsers and the parameter-offset
+remap in turn and requiring `run_com_exec.py t36_argclobber` red. That is the
+first of the next steps, and until it exists the count above does not cover it.
+
 Eight arrived with the previous milestone (`v-TP3-CMDRUN`), and each one is the
 same shape: code that compiled clean and passed every byte-level check, until it
 was **run**. Two are behavioural (below), two come from the new interpreter, two
 pin the two new grammar rows the helper audit gained for `EmXorAl01`, and two
 are the `R` key's mutation and its restored green.
 
-Ten arrived with this one, and they are a different shape: **a check whose own
+Ten arrived with `v-TP3-IMAGELAYOUT`, and they are a different shape: **a check whose own
 input had never been verified.** `check_framedisp` swept a region built from a
 literal that had drifted, and the region is now two independent readings of the
 file required to agree — so the first three cases break each reading in turn:
@@ -1054,21 +1068,13 @@ Details that are deliberate, not incidental:
 
 ## Honest limitations
 
-- **A multi-argument call whose earlier argument is a computed value is still
-  wrong, and is not claimed to be fixed.** `SaveLeft` parks a kind-2 operand
-  (a value that exists only in `AX`) across the parse of the *other* operand of
-  a binary operator, which is what fixed `(p > q) or (q > p)`. The three call
-  parsers never park an argument that has already been parsed, so in
-  `f (a > b, x)` the parse of `x` overwrites `a > b`'s value before the call is
-  emitted, and `f (a > b, c > d)` passes the second comparison twice.
-  Reproduced exactly as written here; the fix belongs to the call path and was
-  out of scope for the operator fix.
-- **`runtest.py` runs one fixture through `R`, not 33.** It drives `t34_arith`
+- **`runtest.py` runs one fixture through `R`, not the whole suite.** It drives
+  `t34_arith`
   through the pty because a pty test is expensive and this one's job is to
   prove the *path* exists (compile → poke → run → report → no file written),
-  which it does with eight assertions. The other 32 are covered by
+  which it does with eight assertions. The other 33 are covered by
   `run_exec86.py`, which calls the same interpreter directly.
-- **Every fixture that compiles is now also run.** 33 of 36 execute; the 3 that
+- **Every fixture that compiles is now also run.** 34 of 37 execute; the 3 that
   do not are `t14` and `t25` (`ENoLib`, by design) and `uierror` (a deliberate
   syntax error). The gap this replaces was nine fixtures that compiled and were
   *never executed* — `t08` const, `t09` if/then/else, `t10` while, `t11` for/to,
@@ -1083,13 +1089,15 @@ Details that are deliberate, not incidental:
   execution check to go red. Finding them cost four fixes; see the milestone
   section.
 - **A call takes at most 16 arguments, and says "compiler overflow" if you
-  exceed it.** `args` is `ARRAY [0..15] OF ERes` in the three call parsers, and
-  the guard raises `ECompOvf` = 99. So `far (1, 2, ... , 70)` is rejected with
+  exceed it.** Each of the three call parsers counts as it parses and raises
+  `ECompOvf` = 99 at the seventeenth — there is no `args` array left to
+  overflow, since an argument is pushed the moment it has been parsed. So
+  `far (1, 2, ... , 70)` is rejected with
   error 99, whose text in TP3 means the compiler's own table overflowed — an
   error about the compiler, for a program that merely has a long argument
   list. This is why `t28_farparam` can *declare* 70 parameters (which is what
-  puts `p63` at `[BP+128]` and exercises the disp16 encoding) but can only
-  *pass* 16, and why its body reads `p63`/`p70` into a variable whose value is
+  puts `p8` at `[BP+128]` and exercises the disp16 encoding) but can only
+  *pass* 16, and why its body reads `p8`/`p1` into a variable whose value is
   deliberately absent from the `.out`: those two slots hold stack garbage, and a
   fixture that printed them would be testing the harness, not the compiler.
 - **Parameters are separated by `,` and only by `,`.** `procedure two (a : integer ; b : integer)`
@@ -1598,10 +1606,12 @@ reason for existing.
     shape table *is* the fix — an inline "just reload it" at one call site
     would not survive the next operator.
 
-    This bug is also why the fix has a stated boundary: the **call** path has
-    the same hole and does not have it fixed (`f (a > b, x)`). A fix that
-    claims "computed operands" while only covering binary operators is worse
-    than one that writes its edge down; see Honest limitations.
+    This bug is also why the fix had a stated boundary: the **call** path had
+    the same hole (`f (a > b, x)`) and was written down as unfixed rather than
+    quietly left out of a claim about "computed operands". A fix that claims
+    the general case while covering one call site is worse than one that writes
+    its edge down. That boundary is closed by bug 36 below, and the fixture
+    that closes it is `t36_argclobber`.
 
 35. **`not` was lowered identically for booleans and integers, so every
     boolean negation was wrong.** TPSRC9's `neglevel` picks the instruction
@@ -1627,9 +1637,68 @@ reason for existing.
     claims — moving the `34H` to `35H` fails with *"no name pattern accepts
     it"*, so the emitter cannot silently become a different instruction.
 
+### Then a computed argument met the parse of the next one, and one more appeared
+
+36. **Every call parser read the whole argument list before pushing any of it,
+    so an argument that existed only in `AX` did not survive the parse of the
+    argument after it — and the callee numbered its parameter slots the opposite
+    way round from the pushes.** `t36_argclobber` exists because no fixture had
+    ever called anything with a computed argument anywhere but last.
+
+    Three parsers deferred the pushes: `ParseCallArgs` (a procedure call as a
+    statement), `ParseCall` (a function called inside an expression) and
+    `IoCall` (`write`/`writeln`/`read`). A `kind 2` result means the value is in
+    `AX` and nowhere else, and `LoadAtom` deliberately does nothing to it — so by
+    the time a deferred loop reached argument *i*, `AX` held whatever argument
+    *i+1* had computed. `p2 (a + b, x)` printed `0 0`, `b2 (a > b, x > c)`
+    printed `FALSE FALSE`, and `writeln (add2 (c * 2, a))` printed `10` instead
+    of `19`. `IoCall` carried a second loss underneath that one: it pushed after
+    the *whole* list had been parsed, so an argument was also pushed after the
+    runtime call made for an inline string literal in between —
+    `writeln (b > a, ' ', x + 1)` printed `TRUE 544`, the first argument handed
+    over as the third one's value and the third read out of an `AX` that a call
+    had already had. `read`/`readln` could not hit any of this, because their
+    arguments are always plain names and a name emits nothing while it is
+    parsed.
+
+    The other half was the callee. Parameter offsets were handed out in
+    declaration order from `BP+4`, so the *first* declared parameter sat at
+    `BP+4`; but an argument pushed first ends up farthest from `BP`, so the two
+    halves disagreed about which parameter was where, and a one-argument call
+    only ever worked because one slot and one push cannot disagree. Both halves
+    now follow the original: TPSRC8 `cproc`/`cprlp1`/`cprlp2` emits
+    `CALL exprsave` then `CALL epushax` for each argument *as it is read* and
+    only then the `CALL`, and RESUME-TP3.md §3.11 states that the last declared
+    parameter is the one at `BP+4` — which is exactly where the first-pushed
+    argument ends up. `Runtime.mod` needed no change: every entry it has takes
+    one stack argument, and one push and one slot are order-independent.
+
+    So each argument is loaded and pushed the moment it has been parsed, in
+    parse order; `IoCall`'s parse loop and its emit loop are one loop; and once
+    a procedure's parameter list has been read, its symbols are remapped with
+    `off := parmOff + 2 - off`, which sends `4 + 2*(k-1)` to `4 + 2*(n-k)` and
+    invents no offset in between. The range that remap sweeps is exactly the
+    parameters: the parameter's own `NewSym` inside the loop is the only symbol
+    created there, `ParseType` creates none, the procedure's own name predates
+    `nestMark`, and a function's result variable comes after it.
+
+    No expectation row moved, and that is worth as much as the fix: the reorder
+    emits the same bytes in a different order, and the remap hands the same set
+    of displacements to different names — `t28`'s two disp16 reads are still at
+    `+128` and `+142`, now read through `p8` and `p1` where they were `p63` and
+    `p70`, and its printed sum still comes from the two slots the sixteen
+    pushed arguments land in. So nothing was re-baselined; the `t36` row was
+    written while the compiler was still broken, and its numbers simply held.
+
+    The red was seen before the fixture was encoded, not after: the pre-fix
+    compiler got the two plain-name lines right and the other six wrong, on
+    both oracles at once. Those two greens matter — they are what would catch a
+    fix that flipped only one of the two halves — and they were green *before*
+    the change as well as after it.
+
 ## The bug family, stated once
 
-Nine of the thirty-five are the *same* bug in different clothes: **loading the
+Nine of the bugs recorded here are the *same* bug in different clothes: **loading the
 address where the value was wanted, or picking the register one byte or one
 letter away from the right one.** `EmPushVarAddr` had the right bytes for the
 wrong register. `LdAlBx` and `MovAlBl` are one letter apart. `MovAh0` and
@@ -1706,11 +1775,14 @@ independently-scanned inventory at all.
 
 ## Next steps
 
-1. **The multi-argument kind-2 clobber.** `f (a > b, x)` passes a wrong first
-   value, and `f (a > b, c > d)` passes the second comparison twice.
-   `SaveLeft` parks a computed operand across the parse of the *other* operand
-   of a binary operator; the three call parsers park nothing. Fixture first, so
-   the bug is red before the fix (see "Honest limitations").
+1. **The in-suite non-vacuity cases for bug 36.** `t36_argclobber`'s `.out`
+   was proved able to fail by running the pre-fix compiler — red on both
+   oracles, quoted in the commit that introduced the fixture — but
+   `nonvacuity.sh` does not yet mutate the fix *back*. Revert `ParseCallArgs`,
+   `ParseCall`, `IoCall` and the parameter-offset remap in turn, and require
+   `run_com_exec.py t36_argclobber` red for the stated reason, green on
+   restore. Until that exists, the `62 ok` under "Non-vacuity" does not cover
+   this fixture, and the text there says so.
 2. **String *variables*** — `s : string`, `s := 'hi'`, `writeln(s)`. The
    encoding blocker is gone (`EmBpDisp`); what is left is a length word, an
    assignment path, and a `WrStr` entry (TPSRC4 `xwrtstr`). `IoCall` currently

+ 143 - 114
shell/Compiler.mod

@@ -276,7 +276,9 @@ VAR
 
    locFree  : CARDINAL ;       (* next local slot (BP-relative, 8-bit) *)
    locBytes : CARDINAL ;       (* frame size for SUB SP *)
-   parmOff  : CARDINAL ;       (* next parameter slot (BP-relative) *)
+   parmOff  : CARDINAL ;       (* provisional parameter slot, in declaration
+                                  order; remapped to TP3's BP+4 rule when the
+                                  list has been read (see ProcFunc) *)
 
    dirs     : DirRec ;
 
@@ -2149,10 +2151,19 @@ BEGIN
 END EmCallMost ;
 
 PROCEDURE ParseCallArgs (idx : CARDINAL) ;
-(* '(' already consumed: read args ')' then call.  Arguments are pushed
-   right-to-left so the first-declared parameter lands at BP+4. *)
-VAR args : ARRAY [0..15] OF ERes ;
-    nArgs, i : CARDINAL ;
+(* '(' already consumed: read args ')' then call.
+
+   Each argument is loaded and pushed the moment it has been parsed, which is
+   what TPSRC8 cproc/cprlp1/cprlp2 do - CALL exprsave, CALL epushax, one per
+   argument, then the CALL.  A computed argument (kind 2) exists only in AX,
+   so parsing the argument after it would destroy it before the push loop
+   ever reached it: `p (a > b, x)' passed x twice.
+
+   Pushing in parse order is also the order the callee is laid out for: the
+   first argument goes on first and so ends up farthest from BP, and the LAST
+   declared parameter is the one at BP+4 (RESUME-TP3.md 3.11). *)
+VAR arg : ERes ;
+    nArgs : CARDINAL ;
 BEGIN
    nArgs := 0 ;
    IF CurCh () = ')' THEN
@@ -2163,7 +2174,9 @@ BEGIN
             Err (ECompOvf) ;
             EXIT
          END ;
-         ParseExpr (args [nArgs]) ;
+         ParseExpr (arg) ;
+         LoadAtom (arg) ;
+         EmPushAx () ;
          INC (nArgs) ;
          IF NOT MatchDelim (',') THEN
             EXIT
@@ -2171,19 +2184,14 @@ BEGIN
       END ;
       ExpectDelim (')', ENoSemi)
    END ;
-   i := nArgs ;
-   WHILE i > 0 DO
-      DEC (i) ;
-      LoadAtom (args [i]) ;
-      EmPushAx ()
-   END ;
    EmCallMost (idx, nArgs)
 END ParseCallArgs ;
 
 PROCEDURE ParseCall (idx : CARDINAL) ;
-(* procedure/function call; '(' optional *)
-VAR args : ARRAY [0..15] OF ERes ;
-    nArgs, i : CARDINAL ;
+(* procedure/function call; '(' optional - same parse-then-push as above,
+   reached from ParseAtom when a FUNCTION is called in an expression *)
+VAR arg : ERes ;
+    nArgs : CARDINAL ;
 BEGIN
    nArgs := 0 ;
    IF MatchDelim ('(') THEN
@@ -2193,7 +2201,9 @@ BEGIN
                Err (ECompOvf) ;
                EXIT
             END ;
-            ParseExpr (args [nArgs]) ;
+            ParseExpr (arg) ;
+            LoadAtom (arg) ;
+            EmPushAx () ;
             INC (nArgs) ;
             IF NOT MatchDelim (',') THEN
                EXIT
@@ -2204,12 +2214,6 @@ BEGIN
          DropCh (GetCh ())
       END
    END ;
-   i := nArgs ;
-   WHILE i > 0 DO
-      DEC (i) ;
-      LoadAtom (args [i]) ;
-      EmPushAx ()
-   END ;
    EmCallMost (idx, nArgs)
 END ParseCall ;
 
@@ -2314,8 +2318,8 @@ PROCEDURE IoCall (idx : CARDINAL) ;
    WRITE/WRITELN push the value; READ/READLN push the address, so the
    runtime can store.  As everywhere else in this compiler the caller
    cleans the argument off the stack. *)
-VAR args : ARRAY [0..15] OF ERes ;
-    nArgs, i, ent, which, acls : CARDINAL ;
+VAR arg : ERes ;
+    nArgs, ent, which, acls : CARDINAL ;
     reading : BOOLEAN ;
     pushed : BOOLEAN ;
     n : CARDINAL ;
@@ -2339,7 +2343,98 @@ BEGIN
                Err (ECompOvf) ;
                EXIT
             END ;
-            ParseExpr (args [nArgs]) ;
+            ParseExpr (arg) ;
+            (* One argument at a time: parse it, give it its own push or its
+               own runtime call, and only then read the next one.  TPSRC8
+               pwrloop does exactly this, and it has to - a computed argument
+               exists only in AX, so parsing the argument after it, and the
+               call made for THIS one, are both free to destroy it.  Reading
+               the whole list first and emitting afterwards printed the first
+               argument as the last argument's value, and pushed an argument
+               that an earlier inline string's call had already overwritten. *)
+            pushed := TRUE ;      (* default: value is on the stack -> call + pop *)
+            IF reading THEN
+               IF arg.kind # 1 THEN
+                  Err (ETypeErr) ;                  (* READ needs a variable *)
+                  RETURN
+               END ;
+               acls := symtab [arg.idx].cls ;
+               IF acls = TString THEN
+                  Err (ENoLib) ;                    (* string runtime pending *)
+                  RETURN
+               END ;
+               EmPushVarAddr (symtab [arg.idx].local, symtab [arg.idx].off) ;
+               IF acls = TReal THEN
+                  ent := TU_RdInt                   (* real reads: not yet *)
+               ELSIF acls = TBool THEN
+                  ent := TU_RdBool
+               ELSIF acls = TChar THEN
+                  ent := TU_RdChar                  (* one byte, not a word *)
+               ELSIF acls = TScalar THEN
+                  ent := TU_RdInt
+               ELSE
+                  Err (ETypeErr) ;
+                  RETURN
+               END
+            ELSE
+               acls := arg.cls ;
+               IF (acls = TString) AND (arg.kind = 3) THEN
+                  (* An inline string literal.  TP3 TPSRC8 pwrinlin special-cases a
+                     literal that is followed directly by ',' or ')' - i.e. an
+                     argument, not an expression - and emits
+                        CALL wrtinl  <length byte> <characters...>
+                     with no stack argument at all; wrtinl reads the length from the
+                     return address and returns to just past the last character.
+                     Mirrored exactly, so the literal costs only its own characters
+                     in the code stream and nothing in the data segment. *)
+                  IF strLen [arg.strx] > 255 THEN
+                     (* The length is one byte, so a literal of 256 characters or
+                        more would wrap: 300 characters emitted behind a length of
+                        44, and the runtime would print 44 of them and silently drop
+                        the rest.  TP3 strings are at most 255 characters, so refuse
+                        rather than truncate. *)
+                     Err (EConstRange) ;
+                     RETURN
+                  END ;
+                  DropC (EmCall (TU_WrInl)) ;
+                  Ebyte (VAL (BYTE, strLen [arg.strx])) ;
+                  n := 0 ;
+                  WHILE n < strLen [arg.strx] DO
+                     Ebyte (VAL (BYTE, ORD (strPool [strOff [arg.strx] + n]))) ;
+                     INC (n)
+                  END ;
+                  pushed := FALSE                (* nothing was pushed for this one *)
+               ELSE
+                  IF acls = TString THEN
+                     (* A string *variable*.  Not emitted rather than emitted wrongly:
+                        EmPushVarAddr's local form is still wrong (see the note on
+                        that procedure), and a wrong address here would print
+                        garbage instead of failing. *)
+                     Err (ENoLib) ;
+                     RETURN
+                  END ;
+                  LoadAtom (arg) ;
+                  EmPushAx () ;
+                  IF arg.chr THEN
+                     ent := TU_WrChar                (* 'a' - one char, not 97 *)
+                  ELSIF acls = TReal THEN
+                     ent := TU_WrReal
+                  ELSIF acls = TBool THEN
+                     ent := TU_WrBool
+                  ELSIF acls = TChar THEN
+                     ent := TU_WrChar                (* c : char - one char *)
+                  ELSIF acls = TScalar THEN
+                     ent := TU_WrInt
+                  ELSE
+                     Err (ETypeErr) ;
+                     RETURN
+                  END
+               END
+            END ;
+            IF pushed THEN
+               DropC (EmCall (ent)) ;
+               EmAddSp (2)                         (* one 16-bit argument *)
+            END ;
             INC (nArgs) ;
             IF NOT MatchDelim (',') THEN
                EXIT
@@ -2358,95 +2453,6 @@ BEGIN
       DropC (EmCall (TU_RdLn)) ;
       RETURN
    END ;
-   (* NB: guard the loop bound - with nArgs = 0, "nArgs - 1" would wrap round
-      to 65535 in CARDINAL and spin 65536 times. *)
-   IF nArgs > 0 THEN
-      FOR i := 0 TO nArgs - 1 DO
-      pushed := TRUE ;      (* default: value is on the stack -> call + pop *)
-      IF reading THEN
-         IF args [i].kind # 1 THEN
-            Err (ETypeErr) ;                  (* READ needs a variable *)
-            RETURN
-         END ;
-         acls := symtab [args [i].idx].cls ;
-         IF acls = TString THEN
-            Err (ENoLib) ;                    (* string runtime pending *)
-            RETURN
-         END ;
-         EmPushVarAddr (symtab [args [i].idx].local, symtab [args [i].idx].off) ;
-         IF acls = TReal THEN
-            ent := TU_RdInt                   (* real reads: not yet *)
-         ELSIF acls = TBool THEN
-            ent := TU_RdBool
-         ELSIF acls = TChar THEN
-            ent := TU_RdChar                  (* one byte, not a word *)
-         ELSIF acls = TScalar THEN
-            ent := TU_RdInt
-         ELSE
-            Err (ETypeErr) ;
-            RETURN
-         END
-      ELSE
-         acls := args [i].cls ;
-         IF (acls = TString) AND (args [i].kind = 3) THEN
-            (* An inline string literal.  TP3 TPSRC8 pwrinlin special-cases a
-               literal that is followed directly by ',' or ')' - i.e. an
-               argument, not an expression - and emits
-                  CALL wrtinl  <length byte> <characters...>
-               with no stack argument at all; wrtinl reads the length from the
-               return address and returns to just past the last character.
-               Mirrored exactly, so the literal costs only its own characters
-               in the code stream and nothing in the data segment. *)
-            IF strLen [args [i].strx] > 255 THEN
-               (* The length is one byte, so a literal of 256 characters or
-                  more would wrap: 300 characters emitted behind a length of
-                  44, and the runtime would print 44 of them and silently drop
-                  the rest.  TP3 strings are at most 255 characters, so refuse
-                  rather than truncate. *)
-               Err (EConstRange) ;
-               RETURN
-            END ;
-            DropC (EmCall (TU_WrInl)) ;
-            Ebyte (VAL (BYTE, strLen [args [i].strx])) ;
-            n := 0 ;
-            WHILE n < strLen [args [i].strx] DO
-               Ebyte (VAL (BYTE, ORD (strPool [strOff [args [i].strx] + n]))) ;
-               INC (n)
-            END ;
-            pushed := FALSE                (* nothing was pushed for this one *)
-         ELSE
-            IF acls = TString THEN
-               (* A string *variable*.  Not emitted rather than emitted wrongly:
-                  EmPushVarAddr's local form is still wrong (see the note on
-                  that procedure), and a wrong address here would print
-                  garbage instead of failing. *)
-               Err (ENoLib) ;
-               RETURN
-            END ;
-            LoadAtom (args [i]) ;
-            EmPushAx () ;
-            IF args [i].chr THEN
-               ent := TU_WrChar                (* 'a' - one char, not 97 *)
-            ELSIF acls = TReal THEN
-               ent := TU_WrReal
-            ELSIF acls = TBool THEN
-               ent := TU_WrBool
-            ELSIF acls = TChar THEN
-               ent := TU_WrChar                (* c : char - one char *)
-            ELSIF acls = TScalar THEN
-               ent := TU_WrInt
-            ELSE
-               Err (ETypeErr) ;
-               RETURN
-            END
-         END
-      END ;
-      IF pushed THEN
-         DropC (EmCall (ent)) ;
-         EmAddSp (2)                         (* one 16-bit argument *)
-      END
-      END
-   END ;
    IF which = BI_WriteLn THEN
       DropC (EmCall (TU_WrLn))
    ELSIF which = BI_ReadLn THEN
@@ -3147,6 +3153,29 @@ BEGIN
          DropCh (GetCh ())
       END
    END ;
+   (* TP3 lays a parameter list out BACKWARDS from BP+4: the LAST declared
+      parameter sits at BP+4 and each earlier one is 2 bytes farther from BP
+      (RESUME-TP3.md 3.11 - "le dernier paramètre déclaré est le plus proche
+      (BP+4)" - and the Code Generation Internals stack frame, "BP+4: last
+      parameter").  This is the other half of pushing the arguments as they
+      are parsed: the first argument is pushed first and so ends up farthest
+      from BP, which has to be the FIRST declared parameter's slot.
+
+      During the loop the offsets were provisional, in declaration order
+      (parmOff starts at 4 and grows by 2 per parameter), so parameter k of n
+      sat at 4 + 2*(k-1).  The map `off := parmOff + 2 - off' sends that to
+      4 + 2*(n-k) - k = 1 to the far end, k = n to BP+4 - which is the layout
+      above, with no offset invented and none lost.
+
+      The range is exactly the parameters: the only symbol created while the
+      list is read is the parameter itself (NewSym inside the loop),
+      ParseType creates no symbols, the procedure's own name predates
+      nestMark, and a function's result variable is created after this point. *)
+   i := nestMark ;
+   WHILE i < symTop DO
+      symtab [i].off := parmOff + 2 - symtab [i].off ;
+      INC (i)
+   END ;
    IF isFunc THEN
       IF MatchDelim (':') THEN
          ParseType (cls, size, elem)

+ 38 - 28
shell/tests/check_framedisp.py

@@ -24,21 +24,23 @@ therefore accidentally correct across -32768..+127, which is where almost
 every variable lives, and every existing fixture kept its exact bytes.
 
 It went wrong at +128: disp8 80h is -128 and not +128, so a read of
-[BP+128] became a read of [BP-128].  Procedure PARAMETERS are laid out
-upward from BP+4, so the 63rd parameter of a procedure is at 4 + 62*2 = 128
-and the 63rd word of every call's argument block was read from the wrong
-side of BP.  Nobody had hit it because no fixture declared that many
-parameters, and because a program with no local variables has no BP-relative
-access at all -- so the whole BP path had zero coverage.
+[BP+128] became a read of [BP-128].  Procedure parameters are laid out from
+BP+4 and grow AWAY from it - the last declared parameter is the one at BP+4,
+which is what makes it the counterpart of pushing arguments as they are
+parsed - so with seventy of them declared the 8th parameter sits at
+4 + 62*2 = 128, the 8th word of a full argument block, and that word was read
+from the wrong side of BP.  Nobody had hit it because no fixture declared that
+many parameters, and because a program with no local variables has no
+BP-relative access at all -- so the whole BP path had zero coverage.
 
 The rule now enforced
 ---------------------
 EmBpDisp picks disp8 for off <= 127 and disp16 otherwise, where `off` is
 taken as a 16-bit value.  Note that every offset above 32767 is *negative* as
 a displacement, so "otherwise" covers all of them; there is no overflow case
-and no 32767 ceiling.  Both fixtures below therefore have every frame access
-encoded in the 4-byte form, and this check requires that form to be present
-and the 3-byte form to be ABSENT for those offsets -- so restoring the old
+and no 32767 ceiling.  The offsets this check asks for below are therefore
+encoded in the 4-byte form, and it requires that form to be present and the
+3-byte form to be ABSENT for those offsets -- so restoring the old
 `off MOD 100H` turns the test red.
 
 The expected offsets are computed here from the frame-layout rules rather
@@ -48,8 +50,9 @@ restatement of what the compiler happens to do:
     locals      locFree starts at 0FFFEh and the NEW SYMBOL IS GIVEN THE
                 PRE-DECREMENT VALUE, so the first local of a procedure is at
                 0FFFEh = -2, the second at 0FFFCh = -4, and so on down.
-    parameters  parmOff starts at 4 and grows by 2 per parameter, so
-                parameter k is at 4 + 2*(k-1).
+    parameters  TP3 lays the list out from BP+4 backwards: the LAST declared
+                parameter is at BP+4 and each earlier one is 2 higher (see
+                RESUME-TP3.md 3.11), so parameter k of n is at 4 + 2*(n-k).
 
 The region this check reads
 ---------------------------
@@ -82,19 +85,21 @@ Fixtures
 t27_localvar.pas  five locals, assigned and read back.  Covers the negative
                   half of the range, which is where every real variable is.
 t28_farparam.pas  seventy declared parameters, of which the call passes
-                  sixteen (the call site caps arguments at 16).  p63 and p70
-                  are at +128 and +142, the first offsets the disp8 form
-                  cannot represent.
+                  sixteen (the call site caps arguments at 16).  Laid out from
+                  BP+4 backwards, p1 is at +142 and p8 at +128: the first
+                  offsets the disp8 form cannot represent.
 
 t28 is executed as well as compiled, and that distinction matters for what its
-.out file may assert.  `g := p16 + p1' is deterministic (1 + 16 = 17) and is
-the printed value.  `unused := p63 + p70' reads stack garbage, because a call
-caps at 16 arguments and p17..p70 are never passed; it is computed and
-discarded, and its value is deliberately NOT in the .out file.  A fixture that
-printed it would be asserting a number that depends on what the caller left on
-the stack, which is a test of the harness rather than of the compiler.  What
-t28 establishes for p63/p70 is the ENCODING, which is the thing that was
-wrong.  Do not read t28 as a claim that a 70-argument call works.
+.out file may assert.  `g := p55 + p70' is deterministic (1 + 16 = 17) and is
+the printed value: with only sixteen arguments pushed, they cover BP+4..BP+34,
+and those are the slots p55..p70 sit in - the last declared parameter is the
+one at BP+4.  `unused := p8 + p1' reads stack garbage, because a call caps at
+16 arguments and p1..p54 are never passed; it is computed and discarded, and
+its value is deliberately NOT in the .out file.  A fixture that printed it
+would be asserting a number that depends on what the caller left on the stack,
+which is a test of the harness rather than of the compiler.  What t28
+establishes for p1/p8 is the ENCODING, which is the thing that was wrong.
+Do not read t28 as a claim that a 70-argument call works.
 
 Usage: check_framedisp.py [-v]     (from shell/)
 """
@@ -134,9 +139,11 @@ def expected_local_offsets(count):
     return [-(2 + 2 * i) for i in range(count)]
 
 
-def expected_param_offsets(index):
-    """parmOff starts at 4 and grows by 2 per parameter."""
-    return 4 + 2 * (index - 1)
+def expected_param_offsets(count, index):
+    """TP3 puts the LAST declared parameter at BP+4 and walks backwards from
+    there, so `index` (1-based) of `count` declared is at 4 + 2*(count-index):
+    parameter `count` at +4, parameter 1 at +4 + 2*(count-1)."""
+    return 4 + 2 * (count - index)
 
 
 CASES = [
@@ -145,9 +152,12 @@ CASES = [
     # (load).
     ("t27_localvar", "89", expected_local_offsets(5)),
     ("t27_localvar", "8B", expected_local_offsets(5)),
-    # t28: `unused := p63 + p70` -- read but never printed, see the note above.
-    ("t28_farparam", "8B", [expected_param_offsets(63),
-                            expected_param_offsets(70)]),
+    # t28: `unused := p8 + p1` -- read but never printed, see the note above.
+    # p8 is the first parameter whose slot a disp8 cannot reach (+128) and p1
+    # is the farthest of the seventy (+142); the printed reads (p55, p70) are
+    # inside the passed argument block and stay disp8.
+    ("t28_farparam", "8B", [expected_param_offsets(70, 8),
+                            expected_param_offsets(70, 1)]),
 ]
 
 

+ 19 - 8
shell/tests/fixtures/expected.tsv

@@ -81,14 +81,16 @@
 #                 encodings address the same place; only the rule for choosing
 #                 between them changed.
 #   t28_farparam  70 declared parameters; the call passes 16, which is the
-#                 argument cap at the call site.  p63 lands at BP+128 and p70
-#                 at BP+142 - the first offsets a disp8 cannot represent,
-#                 since 80h is -128 and not +128.  Under the old code these
-#                 two reads came from the wrong side of BP.  This fixture
-#                 tests the ENCODING and is never executed; do not read it as
-#                 a claim that a 70-argument call works.  Its 123 bytes are
-#                 1 more than t27's because the body is one ADD and one
-#                 store rather than five of each.
+#                 argument cap at the call site.  Under TP3's frame rule the
+#                 LAST declared parameter is the one at BP+4, so p8 lands at
+#                 BP+128 and p1 at BP+142 - the first offsets a disp8 cannot
+#                 represent, since 80h is -128 and not +128.  Under the old
+#                 code these two reads came from the wrong side of BP.  This
+#                 fixture tests the ENCODING; it is executed too, but its .out
+#                 carries only the deterministic sum of the two slots the
+#                 sixteen pushed arguments land in (p55 + p70), never the
+#                 stack garbage that p1..p54 read.  Do not read it as a claim
+#                 that a 70-argument call works.
 #
 # RE-BASELINED for the case-label fix, and this is the one re-baseline in this
 # file that is a CORRECTION rather than a new feature.  EmMovAxSp used to emit
@@ -173,6 +175,14 @@
 #                 were covered is the last place to leave one.  m := -1, n := -2
 #                 makes the last line signed: unsigned, -1 > -2 is FALSE, so that
 #                 one line is what tells SETG from a byte compare.
+#
+#   t36_argclobber eight lines of hand-derived output, added BEFORE the fix it
+#                 covers: this row was written while the compiler still lost a
+#                 computed argument, was proved red by running it, and its
+#                 numbers did not move when the fix landed - the fix reorders
+#                 bytes rather than adding them, so the same size came out of a
+#                 different order.  A number that never moved and a behaviour
+#                 that did is the same shape as t30 above.
 
 t01_minimal	OK	29	4
 t02_writeln	OK	38	4
@@ -209,4 +219,5 @@ t32_forexit	OK	138	8
 t33_cmpops	OK	404	12
 t34_arith	OK	448	8
 t35_not	OK	224	7
+t36_argclobber	OK	492	12
 uierror	ERR	41	331

+ 14 - 10
shell/tests/fixtures/t28_farparam.pas

@@ -2,14 +2,18 @@ program t28;
 var
   g : integer ;
   unused : integer ;
-(* Seventy parameters.  p63 and p70 sit at +128 and +142, the first offsets
-   an 8-bit displacement cannot represent, so their accesses must use the
-   4-byte mod=10 form -- that is what check_framedisp.py asserts.
-   A call caps at 16 arguments, so p17..p70 are never passed and reading
-   them returns stack garbage.  `unused' takes that sum and is never
-   printed: the point of the fixture is the ENCODING, and the printed
-   value must stay deterministic.  Do not read this as a claim that a
-   70-argument call works. *)
+(* Seventy parameters, laid out the way TP3 lays them out: the LAST declared
+   one sits at BP+4 and each earlier parameter is 2 bytes farther from BP, so
+   p1 is at +142 and p8 at +128 -- the first offsets an 8-bit displacement
+   cannot represent -- and their accesses must use the 4-byte mod=10 form;
+   that is what check_framedisp.py asserts.
+   A call caps at 16 arguments, so p1..p54 are never passed and reading them
+   returns stack garbage.  `unused' takes that sum and is never printed, so
+   the point of the fixture is the ENCODING and the printed value must stay
+   deterministic.  The 16 arguments that ARE passed land on p55..p70, which
+   is why `g' reads those two: p55 is where the FIRST argument goes and p70
+   is where the LAST one goes, so p55 + p70 = 1 + 16.  Do not read any of
+   this as a claim that a 70-argument call works. *)
 procedure far (p1 : integer,
   p2 : integer,
   p3 : integer,
@@ -81,8 +85,8 @@ procedure far (p1 : integer,
   p69 : integer,
   p70 : integer) ;
 begin
-  g := p16 + p1 ;
-  unused := p63 + p70
+  g := p55 + p70 ;
+  unused := p8 + p1
 end ;
 begin
   far (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16) ;

+ 8 - 0
shell/tests/fixtures/t36_argclobber.out

@@ -0,0 +1,8 @@
+5 3 7
+0 8
+8 0
+8 0 14
+12 0
+TRUE FALSE
+19
+FALSE 1

+ 89 - 0
shell/tests/fixtures/t36_argclobber.pas

@@ -0,0 +1,89 @@
+program t36;
+
+{ t36 -- every argument of a call keeps its own value.
+
+  All three call parsers used to read the whole argument list first and emit
+  the pushes afterwards:
+
+      ParseCallArgs   `p (a, b)' as a statement
+      ParseCall       a function call inside an expression
+      IoCall          write/read, one call per argument
+
+  A computed argument (kind 2) has already been emitted and lives only in AX,
+  so once the parser had moved on to the NEXT argument, AX belonged to
+  whatever that argument computed and the earlier value was gone by the time
+  the push loop reached it.  TPSRC8 cproc/cprlp1 does it the other way round
+  -- `CALL exprsave', then `CALL epushax', for each argument as it is parsed,
+  and then a CALL, which is why the original cannot lose one.
+
+  So: parse one argument, put it on the stack, parse the next.  The callee
+  follows the same ground truth from the other end (RESUME-TP3.md 3.11, and
+  the Code Generation Internals stack frame): the LAST declared parameter is
+  the one at BP+4, because the first declared is pushed first and so ends up
+  farthest away.
+
+  Every line below is derived by hand from the Pascal.  The parameters are
+  u, v, w in declaration order, and a=5, b=3, c=7, x=0 throughout, so each
+  expected line can be written down without running anything:
+
+      p3 (a, b, c)          5 3 7      three plain names: pins the three
+                                        slots against either half of the
+                                        convention being flipped alone
+      p2 (x, a + b)         0 8        computed argument LAST
+      p2 (a + b, x)         8 0        computed argument FIRST: the deferred
+                                        push printed `0 0' here
+      p3 (a+b, x, c*2)      8 0 14     two computed, one named
+      p2 (add2 (a+b, 4), x) 12 0       a call nested in an argument
+      b2 (a > b, x > c)     TRUE FALSE boolean parameters, both computed
+      write add2 (c*2, a)   19         a function call parsed by ParseCall
+      write (b>a, ' ', x+1) FALSE 1    IoCall, two losses at once: deferred
+                                        until the whole list had been read,
+                                        the first argument was pushed as the
+                                        THIRD argument's value (`TRUE') and
+                                        the third was pushed after the earlier
+                                        arguments' runtime calls had already
+                                        had AX (`544' instead of 1)
+
+  READ is not here on purpose: it refuses anything but a plain name, a name
+  emits no code while it is parsed, and there is therefore nothing to lose. }
+
+var
+  a : integer ;
+  b : integer ;
+  c : integer ;
+  x : integer ;
+
+procedure p2 (u : integer, v : integer) ;
+begin
+  writeln (u, ' ', v)
+end ;
+
+procedure p3 (u : integer, v : integer, w : integer) ;
+begin
+  writeln (u, ' ', v, ' ', w)
+end ;
+
+procedure b2 (u : boolean, v : boolean) ;
+begin
+  writeln (u, ' ', v)
+end ;
+
+function add2 (u : integer, v : integer) : integer ;
+begin
+  add2 := u + v
+end ;
+
+begin
+  a := 5 ;
+  b := 3 ;
+  c := 7 ;
+  x := 0 ;
+  p3 (a, b, c) ;
+  p2 (x, a + b) ;
+  p2 (a + b, x) ;
+  p3 (a + b, x, c * 2) ;
+  p2 (add2 (a + b, 4), x) ;
+  b2 (a > b, x > c) ;
+  writeln (add2 (c * 2, a)) ;
+  writeln (b > a, ' ', x + 1)
+end.

+ 4 - 0
v-TP3-DEAD-FIXTURES.md

@@ -54,6 +54,10 @@ Tests / harness
 - Runtime entries: 36 passed / 0 failed.
 - `t28_farparam` references p63/p70 to exercise disp16 encoding but does
   not assert their runtime values (they are unpassed due to the 16-arg cap).
+  *(Later `v-TP3-ARGORDER` milestone: the frame now follows TP3's "last
+  declared parameter at BP+4" rule, so the same two offsets, +128 and +142,
+  are read through `p8` and `p1`; the encoding this row is about is
+  unchanged.)*
 
 Tag placement
 -------------