CocoR/fix.txt

CocoGm2 - CRS.mod Analysis and Fix
===================================

Summary
-------
The file CRS.mod (a Modula-2 scanner generated by Coco/R) failed to compile
with the GNU Modula-2 compiler (gm2). The compilation command was:

    gm2 -fiso -c CRS.mod

The compiler crashed with an internal compiler error (ICE) instead of
reporting a normal error/warning. This document records the analysis and
the fixes applied.

----------------------------------------------------------------------
1. Original symptoms
----------------------------------------------------------------------
The raw compile of CRS.mod produced:

    cc1gm2: internal compiler error: ProcedureSym kind has not yet
    been declared
    <backtrace ending in>
    ...SymbolTable_GetProcedureKind
    ...P2SymBuild_BuildFPSection
    ...FormalParameters -> ... -> ProcedureHeading -> ... -> Declaration

FileIO.mod compiled fine with the same flags, so the problem was specific
to CRS.mod, not to the toolchain in general.

----------------------------------------------------------------------
2. Initial findings during review
----------------------------------------------------------------------
Several suspicious items were noted during the code review:

(a) Forward reference
    GetString() called the function CharAt() which was declared LATER in
    the file. In Modula-2 a procedure must be declared before it is used.

(b) Unused local ORDL wrapper
    A module-level procedure `ORDL(n: INT32): CARDINAL` wrapped
    FileIO.ORDL(), but the rest of the code called FileIO.ORDL() directly,
    so the wrapper was dead code.

(c) Nested procedures inside Get()
    Get() contained two nested procedures:
        - Equal(s: ARRAY OF CHAR): BOOLEAN
        - CheckLiteral
    Nested procedures with open-array (ARRAY OF CHAR) parameters were a
    suspected compiler-bug trigger.

(d) CurrentCh procedure-type variable
    CurrentCh was declared as type GetCH = PROCEDURE (INT32): CHAR and was
    (re)assigned to CharAt() in the module initialization block.

----------------------------------------------------------------------
3. Isolation (minimization)
----------------------------------------------------------------------
The ICE was reproduced and reduced to a minimal case:

    IMPLEMENTATION MODULE CRS2;
    IMPORT FileIO;
    ...
    PROCEDURE ORDL (n: INT32): CARDINAL;
      BEGIN
        RETURN FileIO.ORDL(n)
      END ORDL;
    ...

Results of the reductions:
    - Removing Err()          -> still crashed
    - Removing NextCh()       -> still crashed
    - Keeping ONLY the ORDL procedure -> STILL crashed
    - Renaming "ORDL" to "ORDLX"       -> compiled cleanly

CONCLUSION: the crash was caused by DECLARING a module-level procedure
with the SAME NAME as an imported procedure from FileIO
(i.e. `ORDL` shadowing `FileIO.ORDL`). gm2 trips over the name clash and
hits "ProcedureSym kind has not yet been declared" while building the
formal parameter section of surrounding/affected procedures.

The earlier hypothesis (nested procedures, procedure-type variables,
open-array parameters) was explored but is NOT the cause of the ICE; those
constructs compiled fine in isolation.

----------------------------------------------------------------------
4. Fixes applied to CRS.mod
----------------------------------------------------------------------
(1) Removed the unused module-level `ORDL` wrapper procedure. This is the
    actual fix for the compiler crash. All remaining references already
    use the fully-qualified FileIO.ORDL(), which is correct.

(2) Restored `CurrentCh: GetCH;` in the VAR section (it had been lost
    from the file during earlier save/stash actions). CurrentCh is used
    by NextCh(), Equal() and CheckLiteral().

(3) Moved CharAt() and CapChAt() BEFORE GetString()/GetName() to resolve
    the forward reference of CharAt() from GetString().

(4) Extracted the nested procedures Equal() and CheckLiteral() out of
    Get() to module level for clarity and to keep away from gm2's
    nested-procedure handling. CheckLiteral now takes sym as a VAR
    parameter and the call site was updated to CheckLiteral(sym).

----------------------------------------------------------------------
5. Verification
----------------------------------------------------------------------
After the fixes the module compiles cleanly:

    gm2 -fiso -c CRS.mod        (no errors, no warnings)

Note: the git diff for CRS.mod appears large because the original file
used CRLF line endings while the edited file uses LF; the real functional
change is the four items listed in section 4.

----------------------------------------------------------------------
6. Whole-project build (./build.sh)
----------------------------------------------------------------------
After CRS.mod compiled, ./build.sh was run. Two further problems were
found and fixed. Both were exposed only during the final link step, in
which gm2 links with the scaffold flags "-fscaffold-dynamic
-fscaffold-main" and re-checks the imported implementation modules.

6.1 gm2 false "s already declared" error in CRA.mod (MatchDFA)
    During the link step gm2 reported:

        cc1gm2: error: In procedure « MatchDFA »: symbol « s » is
        already declared in this scope, use a different name or remove
        the declaration
        ./CRA.mod:659:7: error: symbol « s » also declared in this module
        659 |       s, to: INTEGER (* State *);

    Analysis:
      - CRA.mod compiles fine standalone (gm2 -fiso -c CRA.mod).
      - CR.mod compiles fine standalone (gm2 -fiso -c CR.mod).
      - The error appears ONLY when compiling CR.mod with the scaffold
        flags (as gm2 does during linking):
            gm2 -fiso -fscaffold-dynamic -fscaffold-main -c CR.mod
      - The scaffolding re-parses the imported implementation module
        CRA.mod, and a procedure-local variable "s" from some earlier
        scope leaks into MatchDFA's scope, so the local declaration of
        "s" is flagged as a duplicate. This is a gm2 scope-tracking bug,
        not a real error in the source.
      - Renaming the local "s" in procedures such as TheState() did NOT
        help; renaming the "s" inside MatchDFA() itself does.

    Fix applied to CRA.mod:
      Renamed MatchDFA()'s local variable "s" to "st" (declaration and
      all 7 uses inside the procedure). This avoids the collision with
      the leaked symbol while keeping the code identical in behaviour.

6.2 build.sh errors in the module list
    Once compilation and linking succeeded past the CRA error, the link
    failed twice more because build.sh referenced module object files
    that were either never built or not needed:

      - "CRQ.o" was in the link line but is never compiled. CRQ.mod is a
        legacy alternate MAIN module (not CR); CR.mod does not import
        CRQ.  -> removed CRQ.o from the link line.
      - "CRX.o" was built by the script but was missing from the link
        line, yet CR.mod calls CRX.GenCompiler and CRX.WriteStatistics.
        -> added CRX.o to the link line.

    Final link command in build.sh:
        gm2 -fiso -o CR CRA.o CRC.o CRG.o CRP.o CRS.o CRT.o CRX.o \
                          FileIO.o Sets.o CR.mod

----------------------------------------------------------------------
7. Final verification
----------------------------------------------------------------------
    ./build.sh
        Building Coc/R with GNU Modula-2
        Deleting all o files
        compiling the needed modules
        +++++++Modules compiled ++++++++++++++++++++++
        Compiling main module and linking all together
        (exit code 0)

    Produces the executable ./CR.

    Smoke test:
        echo "?" | ./CR
        Coco/R (WinTel) - Compiler-Compiler V1.53
        Released by Pat Terry 17 September 2002
        (COCOR ? gives short help screen)
        Grammar[.atg] ? : File <?.atg> not found.
        Grammar[.atg] ? :

    The executable starts and behaves as expected.

----------------------------------------------------------------------
Files changed (this session)
----------------------------------------------------------------------
    CRS.mod   - removed unused ORDL wrapper; restored CurrentCh; moved
                CharAt/CapChAt before GetString/GetName; extracted
                Equal/CheckLiteral to module level.
    CRA.mod   - renamed MatchDFA local variable s -> st.
    build.sh  - fixed module list for the link step (drop CRQ.o,
                add CRX.o).
    fix.txt   - this file.
----------------------------------------------------------------------
8. IsoFrames adapted to FileIO (ISO build works fully, 2026)
----------------------------------------------------------------------
Goal: make the ISO-flavoured frames (IsoFrames/) regenerate CRS, CRP and
the driver such that the whole build compiles and LINKS with `gm2 -fiso`.

Why it broke before:
  - iso scanner frame emitted `src, lst: IOChan.ChanId` in its DEFINITION
    MODULE, but the support modules CRA/CRT/CRX call `FileIO.WriteString
    (CRS.lst, ...)`, which requires `CRS.lst: FileIO.File`.  The types are
    incompatible (FileIO.File is a POINTER TO FileRec wrapping its own
    hidden ChanId).
  - iso parser frame emitted only `IMPORT <Scanner>;`; the grammar's
    semantic actions (CR.atg) use FileIO.INT32/Long0/Long2/INTL/INT, so the
    generated parser missed the FileIO import.
  - iso compiler/compile2/gpm frames used `Storage.ALLOCATE(nextErr,
    SYSTEM.TSIZE(ErrDesc))` INSIDE a NESTED module (ListHandler).  In ISO
    mode gm2 rejects that: "Storage looks like a module which has not been
    globally imported".  The working pattern (proved with testS3.mod) is to
    import inside the nested module:
        FROM Storage IMPORT ALLOCATE;
        FROM SYSTEM IMPORT TSIZE;
    and then call ALLOCATE(p, TSIZE(T)) unqualified.

Changes to IsoFrames:
  - scanner.frm / scanner2.frm / scannerc.frm
      * DEFINITION: `IMPORT FileIO;` + `INT32 = FileIO.INT32;` +
        `src, lst: FileIO.File;`
      * IMPLEMENTATION: `IMPORT FileIO, Storage;` (scannerc also keeps
        SYSTEM for TSIZE); Reset reads via FileIO.ReadBytes instead of
        IOChan.RawRead:
            read := BlkSize; FileIO.ReadBytes(src, buf[i]^, read);
      * scanner2 uses FileIO.err + FileIO.ReadBytes in its single-buffer
        Reset.
  - parser.frm: `IMPORT -->scanner;` -> `IMPORT -->scanner, FileIO;` so the
    generated parser gets FileIO as well as the scanner's range (matches the
    classic frame behaviour `IMPORT FileIO, CRS;`).
  - compiler.frm / compile2.frm / compiler.gpm (rudimentary drivers):
      * ListHandler nested module now does
            FROM FileIO IMPORT CR, LF, EOF, WriteString, Write, WriteLn,
                                 WriteInt, Long0;
            FROM Storage IMPORT ALLOCATE;
            FROM SYSTEM IMPORT TSIZE;
        and calls ALLOCATE/TSIZE unqualified; the old CONST CR/LF/EOF have
        been removed (they come from FileIO now).
      * main module switched from ProgramArgs/StdChans/SeqFile/TextIO/
        WholeIO to FileIO: FileIO.NextParameter, FileIO.Open(src,...,FALSE),
        FileIO.Open(lst,...,TRUE), FileIO.StdOut/FileIO.err,
        FileIO.WriteString/WriteInt/WriteLn, FileIO.Close.
        NOTE: FileIO.NextParameter is a PROCEDURE with no return value, so
        it must NOT be used in `IF FileIO.NextParameter(...)`; pass a local
        buffer and test `IF buf[0] = 0C`.

Critical gotcha found while linking:
  - gm2 fails during whole-program checking (pass 3) with
        CRP.mod: error: In procedure <TokenFactor>: too many errors in pass 3
    whenever the generated parser contains ACTIVE `...FORWARD;` declarations.
    The classic build avoids this by generating the parser with the
    multipass option:   CR -m CR.atg
    which makes GenForwardRefs (CRX.mod:539) wrap the FORWARD block in
    (* ----- FORWARD not needed in multipass compilers ... ----- *)
  - Symptom was independent of directory/timestamps; only commenting the
    FORWARD block out fixed it.

Verified full ISO build (fresh dir, gm2 -fiso):
    CRFRAMES=<repo>/IsoFrames <repo>/CR -m CR.atg
    # compiles: CRA CRC CRG CRT CRX CRS CRP FileIO Sets  -> all EXIT 0
    gm2 -fiso -o coco CRA.o CRC.o CRG.o CRT.o CRX.o CRS.o CRP.o \
                    FileIO.o Sets.o CR.mod
    # link OK; ./coco CR.atg -> "Parsed correctly"
    Support modules CRA/CRT/CRX remained unchanged (FileIO already).

----------------------------------------------------------------------
9. Pascal build from pascal.atg + gm2 whole-program link bug (2026)
----------------------------------------------------------------------
Goal: generate a Pascal compiler from pascal.atg using the same
FileIO-based frames and link it with gm2 -fiso.

Generation command (CRFRAMES points to directory containing the frames):
    CRFRAMES=<frames-dir> CR -m -C pascal.atg
  - -m  suppresses FORWARD declarations (multipass-safe code)
  - -C  generates the driver (Pascal.mod from compiler.frm)
  Without -C, only PascalS.mod and PascalP.mod are generated;
  Pascal.mod is NOT written (gated on CRT.ddt["C"] at CR.mod:463-466).

The grammar is intentionally not LL(1) so CR always ends with
"Compilation ended with LL(1) errors." — this is expected and harmless;
PascalS/PascalP/Pascal are still written.

Compilation:
    for m in FileIO PascalS PascalP Pascal; do
      gm2 -fiso -c $m.mod
    done
  All four modules compile cleanly (EXIT 0).

Link problem:
    gm2 -fiso -o Pascal PascalS.o PascalP.o FileIO.o Pascal.mod
  fails with:
    RndFile.def:27: error: In definition module RndFile: the constructor
    type is FlagSet and this is different from the constant constant set
    which has a type FlagSet
    (same for TermFile.def:31, ChanConsts.def:33)

Root cause:
  gm2's default link step performs whole-program pass-3 analysis:
  it re-parses ALL imported definition modules (including its own
  bundled m2iso: RndFile.def, TermFile.def, ChanConsts.def) and
  type-checks every constant set-constructor.  When the total number
  of identifiers/symbols in the program exceeds an internal threshold,
  a buffer overflow corrupts the type of FlagSet inside gm2 itself,
  producing the spurious "constructor type is FlagSet" error on the
  m2iso library's own code.

  This is the same class of bug reported on the gm2 mailing list
  (2023-05, msg00039/msg00045, by Michael Riedl, with workaround
  explained by Gaius Mulley).

  Trigger is symbol-count threshold, not a specific procedure:
  adding any sufficiently large dummy procedure to a working module
  causes the same failure; removing any ~50 identifiers from the
  failing module drops below threshold and restores success.

Workaround — two-step link using module list:
    gm2 -fiso -fgen-module-list=modules.lst \
        -o /dev/null PascalS.o PascalP.o FileIO.o Pascal.mod
    gm2 -fiso -fuse-list=modules.lst \
        -o Pascal PascalS.o PascalP.o FileIO.o Pascal.mod
  The first invocation generates the module list (despite printing
  the RndFile error; the list is still written and the error is
  non-fatal).  The second uses the cached list and AVOIDS the
  whole-program re-analysis entirely — link succeeds, binary works.

Verified full end-to-end build (fresh dir):
    cp <repo>/Test/{FileIO.mod,FileIO.def,compiler.frm,parser.frm,
         scanner.frm,compiler.gpm,pascal.atg} <build-dir>/
    CRFRAMES=. CR -m -C pascal.atg
    for m in FileIO PascalS PascalP Pascal; do gm2 -fiso -c $m.mod; done
    gm2 -fiso -fgen-module-list=modules.lst -o /dev/null \
        PascalS.o PascalP.o FileIO.o Pascal.mod
    gm2 -fiso -fuse-list=modules.lst -o Pascal \
        PascalS.o PascalP.o FileIO.o Pascal.mod
    # ./Pascal /tmp/test.pas -> "Parsing Parsed correctly" (exit 0)