Skip to content

feat(retain): CONSTANT, RETAIN and NON_RETAIN [NODE-94] - #222

Open
thiagoralves wants to merge 12 commits into
developmentfrom
feature/NODE-94-retain-variables
Open

feat(retain): CONSTANT, RETAIN and NON_RETAIN [NODE-94]#222
thiagoralves wants to merge 12 commits into
developmentfrom
feature/NODE-94-retain-variables

Conversation

@thiagoralves

Copy link
Copy Markdown
Contributor

Implements the IEC 61131-3 variable qualifiers CONSTANT, RETAIN and NON_RETAIN in the compiler, the way CODESYS does. Lands with paired PRs on openplc-editor, openplc-web and openplc-runtime.

What a qualifier now does

Qualifier Effect
CONSTANT Leaf marked read-only; the debugger refuses writes and forces with status 0x87
RETAIN Leaf joins the retain blob; restored at start as a plain write
NON_RETAIN Today's default, and the way to opt a member back out of a retained container
PERSISTENT Folded into RETAIN — CODESYS's distinction has no analogue here

Qualifiers sit on the var block, so a block can now carry a run of them (MANY2 rather than OPTION), and NON_RETAIN clears the inherited bit rather than merely failing to set it. That is why flags travel down the leaf walk as a parameter and never as shared mutable state: a cleared bit must not leak into the next sibling.

VAR_IN_OUT, VAR_TEMP and VAR_EXTERNAL reject RETAIN — none of them owns storage. VAR_INPUT and VAR_OUTPUT accept it, matching CODESYS.

The retain blob

Per-leaf granularity, addressed by the (arrayIdx, elemIdx) pairs the debug table already uses — no offsetof, no sizeof, nothing that depends on C++ layout. A 14-byte header carries magic, format, layout hash, payload length and crc32; the payload is one tightly packed slot per retained leaf.

The layout hash is FNV-1a over the ordered path|typeTag of every retained leaf — identity of the layout, deliberately not of the program. A body edit keeps retained values; adding, removing, retyping or reordering a retained variable invalidates them. Keying on the project MD5 instead would have discarded retained state on every unrelated edit.

Marshalling lives in iec_retain.hpp, parameterised by read/write/size function pointers so one implementation serves the AVR firmware and the Linux runtime's .so.

Retained function blocks

A retained FB instance retains everything it runs on, not just its interface — including library blocks, whose internals the consuming compilation cannot see or even name. The library compiler flattens each block itself and ships the result in the manifest (LibraryFBEntry.leaves); 224 of 224 blocks across the five bundled archives produce a complete list, OSCAT's 172 included.

Locals surface only when the instance is retained, so the debugger keeps its black-box view everywhere else. An archive built before this format is refused with a message naming the block, rather than silently retaining half of it.

One walk serves both the debug table and the library compiler (leaf-walker.ts); they decide the C++ member name of every entry, and a disagreement there breaks the firmware build with nothing catching it earlier.

Verification

2282 passed | 7 skipped, including compiled round-trips against the real runtime C++ that pack, wipe and restore — covering nested FBs, config globals, GVL structs, arrays, NON_RETAIN opt-outs, corrupt payloads, stale layouts and bad magic.

End-to-end on an SLM-RP4 and a P1AM-100. Two identical TONs differing only in RETAIN, both elapsed, then the program reloaded:

2s after reload   dwell(RETAIN): Q=True  ET=10s  STATE=2  |  loose: Q=False ET=3s160ms

Note

.stlib archives are gitignored build artifacts. prepack runs npm run build and files ships libs/, so every release carries freshly-built archives.

🤖 Generated with Claude Code

https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3

thiagoralves and others added 11 commits August 24, 2026 12:22
A CONSTANT is emitted as a `const` C++ member, but the debug table reaches
every leaf through a C-style `(void*)` cast that silently strips the
qualifier — `static_cast` refuses the same conversion outright. Nothing
then stopped `handle_set` / `handle_write` from writing straight into a
genuinely const object: undefined behaviour, and a flat contradiction of
what CONSTANT means to the person who wrote it.

Nobody had hit this because no editor surface could create a const member
in the first place. The upcoming Flags column (blank / CONSTANT / RETAIN)
changes that on day one, so the gate has to land ahead of it.

Carry the qualifier through to the runtime instead of losing it at the
cast:

  - `Entry._pad` becomes `Entry.flags`, so the gate costs no flash and no
    RAM on any target — the byte was already there for alignment.
  - `LEAF_FLAG_READONLY` is set by debug-table-gen and checked by both
    mutating paths, which return the new `STATUS_READ_ONLY` (0x86, the
    next code free after the licensing FCs). Reads are untouched: watching
    a constant is useful, changing it is not. Unforce is refused too — a
    leaf that could never be forced has no force to clear, and reporting
    OK would claim otherwise.
  - The AVR paths in `read_entry` assemble `Entry` field by field, so they
    read the flags byte explicitly. A missed read there returns 0, which
    reads as "writable" and would defeat the gate on exactly the targets
    with the least room to spare.

The flag is threaded down the leaf walk as an explicit parameter rather
than a shared mutable, because a bit can be cleared partway down a subtree
— nothing does that today, but RETAIN will (a NON_RETAIN member inside a
RETAIN function-block instance), and a mutable would leak the cleared
value into the following sibling. It is OR-ed in at each declaring block,
so a `VAR CONSTANT` inside a function block is gated per instance rather
than only at program level, and CONSTANT structs and arrays propagate to
every field and element.

`DebugMapV2` gains an optional `readOnly` so the editor can hide the force
control instead of offering an action that will be refused. Advisory only
— the runtime is what enforces it, which is what keeps an older editor
build, or an OPC-UA client that never reads the map, safe. `version` stays
at 2 deliberately: the editor's debug-parser rejects anything else
outright, so a bump would break every editor pinned to an older strucpp
release the moment it read a new map.

Refs NODE-94

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3
…y owns

0x86 collides with ModbusDebugResponse.REFUSED_BY_SWITCH in the editor's
shared status-code registry (added there before this branch existed), so
use the actual next-free code instead.
…onstant-gate

fix(debug): refuse writes and forces to CONSTANT variables
…orrectly

Phase 1 of NODE-94. The compiler now reads every IEC retention qualifier a
CODESYS project can carry, and RETAIN is allowed exactly where the standard
allows it.

**NON_RETAIN and PERSISTENT had no tokens.** `NON_RETAIN` lexed as an
Identifier, so `VAR NON_RETAIN x : DINT;` failed with "Expected Colon" on the
line BELOW the qualifier — an imported project died pointing at the wrong
place. Both are tokens now.

NON_RETAIN is the default spelled out, so nothing downstream branches on it. It
is still recorded on the block (`isNonRetain`) rather than dropped, so anything
reproducing source from the AST — the LSP formatter, `--decompile-lib` — keeps
it, and so `RETAIN NON_RETAIN` can be reported as the contradiction it is.

PERSISTENT folds into `isRetain`. CODESYS also keeps a PERSISTENT value across a
program download and this toolchain does not implement that, so the honest
mapping is the weaker guarantee both share; splitting them would claim something
nothing delivers. `docs/IEC_COMPLIANCE.md` now says Partial for PERSISTENT and
describes what NON_RETAIN actually does — the row previously claimed NON_RETAIN
was Supported with no token behind it.

**The qualifier is now a MANY, not an OPTION.** `VAR RETAIN PERSISTENT` is the
form a converted project carries, and IEC allows combinations. Contradictions
are semantic errors with a source span rather than a parser complaint about a
missing END_VAR: RETAIN+CONSTANT (as before), and now RETAIN+NON_RETAIN and
CONSTANT+NON_RETAIN.

**RETAIN in a FUNCTION or METHOD was silently accepted.** Neither has an
instance — a function is re-entered from scratch and a method's locals are stack
slots — so the qualifier had nothing to describe, and the user was left
believing a value survived a power cycle. Now an error. This needed a flag of
its own rather than reusing `scopeType`: a method reports "functionBlock" there
so its located variables are rejected the way an FB's are, and overloading it
would have changed that unrelated rule. The message deliberately names no
scope — the only name in reach is the owning FB's, and RETAIN on the FB's own
VAR is legal, so naming it would read as a contradiction.

**RETAIN on VAR_INPUT / VAR_OUTPUT is now allowed**, per IEC 61131-3 Table 13
and CODESYS. The old rule rejected function blocks that are valid everywhere
else. Two existing tests asserted that restriction; they are inverted rather
than deleted, so it cannot quietly return. VAR_IN_OUT, VAR_TEMP and
VAR_EXTERNAL stay refused — a reference, a transient, and a view onto a
VAR_GLOBAL respectively, none of which owns the storage the qualifier would
describe.

Found while testing, NOT fixed here: the POU var-block AST builder does not map
VAR_EXTERNAL, so a function block's external block arrives as blockType "VAR"
and slips past that rule. Pre-existing — the previous rule listed VAR_EXTERNAL
and was equally ineffective there — and fixing the mapping touches
located-variable validation and the external-resolution pass, so it is tracked
separately. Program-scope VAR_EXTERNAL RETAIN is caught, and there is a test at
that scope.

Full suite: 92 files, 2261 passed / 7 skipped (up 14). No new lint warnings.

Refs NODE-94

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3
…etain-modifiers

feat(modifiers): accept NON_RETAIN and PERSISTENT, and scope RETAIN correctly
…shaller

Phase 2 of NODE-94. Replaces the Phase 2.6 retain scaffolding with something a
runtime can actually use, and adds the marshaller both hosts will share.

**The old descriptor could not work.** Each retained variable was
`{ name, offsetof(Class, member), sizeof(IECVar<T>) }`, and all three fields
were wrong for the job:

  - `sizeof(IECVar<T>)` is the whole wrapper. A DINT measures 12 bytes, not 4,
    and the extra 8 are `forced_` and `forced_value_` — so persisting that
    region carried the debugger's forcing state across a power cycle. Force a
    variable during commissioning, power-cycle, and it comes back forced with
    no debugger attached.
  - `offsetof` on a program class is `offsetof` on a non-standard-layout type
    (it derives from ProgramBase and has virtuals): conditionally supported,
    and it warns. The table also used the UNMANGLED member name while the class
    definition went through `mangleMemberIfNeeded`, so any retained variable
    whose name collided with its own type failed to compile.
  - It could only describe members of a PROGRAM. A retained variable inside a
    function block, or a retained CONFIGURATION global, compiled clean and was
    silently dropped.

**Retained leaves are now addressed by debug-table (arr, elem)** — the same
index the debugger already uses. That pass already walks every leaf, including
nested function-block members, struct fields, array elements and configuration
globals, and already reports each leaf's transport width, so all three problems
above disappear rather than being fixed: values move through
`handle_read` / `handle_write`, which touch the value and never the wrapper.

Selection rides the flags parameter Phase 0 introduced, which is why the
container rules fall out cleanly. `applyBlockFlags` ORs a block's own
qualifiers into whatever it inherited, and NON_RETAIN *clears* the bit — the
one case that makes a shared mutable wrong, since a cleared bit must not leak
into the following sibling. Verified: a retained FB instance retains its whole
subtree, a `VAR RETAIN` inside an FB retains in every instance including
non-retained ones, and a NON_RETAIN member opts out of a retained container two
levels up.

**`retain_layout_hash`** is FNV-1a over the ordered `path|typeTag` of the
retained leaves — identity of the LAYOUT, not of the program. A body edit keeps
retained values; adding, removing, retyping or reordering a retained variable
invalidates them. Keying on the project MD5 would have discarded retained state
on every unrelated edit.

**`iec_retain.hpp` becomes the shared marshaller.** The abstract `RetainStorage`
class it used to hold could never have crossed a `.so` or plugin boundary; it is
replaced by a blob format and a pack/unpack walk parameterised by read/write/size
function pointers, so the Arduino firmware and the v4 daemon share one
implementation and cannot drift. 14-byte header (magic, format, layout hash,
length, crc32) plus values packed in table order — no paths, no indices, no type
tags in the retain region, because that region is the scarce one.

Widths come from `size_of()` at runtime, never from the manifest: a STRING moves
as a fixed 127 bytes regardless of `STRING(20)`, and sizing the payload from
anything else desynchronises it from what the target can read.

`getRetainVars` / `getRetainCount` are KEPT as no-op base slots on ProgramBase
and deliberately no longer overridden. The v4 runtime mirrors that vtable by
position to dispatch `run()` across a `.so` boundary, so removing a slot would
mis-dispatch every program built against a different version.

Verified by building and running the real thing, not just compiling it: pack →
wipe → unpack restores a program local, an inherited FB member, an FB-local
RETAIN inside a NON-retained instance and a configuration global; leaves a
NON_RETAIN member untouched; does not force anything; and refuses a corrupt
payload, a stale layout, bad magic, an empty store and a truncated blob. That
round-trip is now a test.

Four tests asserting the removed table were rewritten rather than deleted. Two
also gained a CONFIGURATION, because retained storage exists in an INSTANCE —
an uninstantiated program has nothing to retain, which the old per-program
table obscured.

Suite: 92 files, 2273 passed / 7 skipped (up 12). No lint errors.

Refs NODE-94

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3
…etain-table

feat(retain): leaf-addressed retain table, blob format and shared marshaller
…ust its interface

A retained library FB kept only what the .stlib manifest exposed — its
inputs, outputs and in-outs. Everything the block actually runs on stayed
behind. A retained TON came back with Q and ET but without STATE,
PREV_IN or START_TIME, so on the next start it saw no rising edge to
explain the values it held and restarted its wait: a block restored into
a configuration it could never have reached by running. That is worse
than not retaining it at all, because it looks like it worked.

The consumer cannot fix this for itself. Two things stop it:

  * Mangling is decided against the DECLARING unit. `mangledMemberName`
    adds a trailing underscore when a member's name matches its own
    type's name AND that type is user-defined, or when it collides with
    a method of an interface the owning FB implements — both answered
    from the library's own AST and symbol tables. A library-internal
    type never reaches the manifest, so a consumer resolves it as "not
    user-defined" and names a member the class does not declare.
    `generated_debug.cpp` then fails to compile and takes the firmware
    build with it.

  * Depth is invisible. A local may be a library-internal STRUCT or
    another FB instance, neither of them exported. Walking only what the
    manifest exports stops at the first one — the same partial retain,
    one level down.

So the library compiler runs the walk itself and writes the answer down:
`LibraryFBEntry.leaves` carries every persistent leaf of one instance,
flattened through structs, arrays and nested instances, each with its
path, its already-mangled C++ expression and its type. All 224 function
blocks across the five bundled archives produce a complete list,
OSCAT's 172 included.

One walk, not two. The flattening moved to `leaf-walker.ts`, shared by
the debug table and the library compiler, because they decide the C++
member name of every entry and had agreed only by copy. `TAG`, the flag
bits and the IEC tables moved with it into `debug-leaf-types.ts` so
neither module has to import the other.

Locals are surfaced only when the instance is retained. The debugger
keeps its black-box view everywhere else — that is the long-standing
contract, and it is what stops a project instantiating a few hundred
OSCAT blocks from growing a debug table several times its useful size.

An archive built before this format cannot be retained, and says so:
compiling `VAR RETAIN t : SomeOldLibFB` now fails with a message naming
the block and the fix. Silently keeping half of it is the behaviour this
commit exists to remove, and falling back to it on old input would leave
the same trap for anyone who has not rebuilt.

`retainBlobSize` joins the debug map so a build can be refused when the
target cannot hold the blob. It matters more now than it did: a retained
TON is 36 bytes, so a 512-byte baremetal cap is reached at about
fourteen of them.

Verified on an SLM-RP4. Two identical TONs, ten-second preset, differing
only in RETAIN; both left to elapse, then the program reloaded. Two
seconds later the retained one was still Q=TRUE ET=10s STATE=2 while the
other had restarted its wait at ET=3s160ms.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3
…ibrary-fb-locals

feat(retain): retain a library FB's internal state, not just its interface [NODE-94 phase 5]
Replaces the flattened `LibraryFBEntry.leaves` with a declarative
`LibraryFBEntry.locals`, in the same shape as `inputs` / `outputs` /
`inouts`. The consumer walks them with the walk it already uses for
user-defined blocks.

WHY THE FLATTENED FORM WAS WRONG
--------------------------------
It was built on a claim that does not survive contact with the data:
that a consuming compilation cannot name a library's internal members,
so the library must pre-compute every leaf. Measured across the five
bundled archives — 224 function blocks, 7,639 leaves — mangling applies
to exactly ZERO of them. The case is real but rare, and paying for it by
flattening everything was the wrong trade:

  ESR_COLLECT            773 leaves  ->  3 local declarations
  plcopen-softmotion     39% of the archive was leaf payload
  oscat-basic            324 KB of leaf payload
  all archives           7,639 leaves  ->  682 declarations

Every user paid that, in every project, whether or not they retained
anything. One entry now describes `buf : ARRAY[0..99] OF REAL` instead of
a hundred. plcopen-softmotion drops 632 KB -> 232 KB; oscat 2.2 MB ->
1.7 MB.

The rare case is carried, not guessed at: `LibraryVarType.cppName` holds
the mangled name when — and only when — the library's own codegen
produced one. Zero occurrences in the bundled archives; a user library
declaring `Tally : Tally` gets `cppName: "TALLY_"`, which is the CODESYS
pattern that made mangling necessary in the first place.

`leaf-walker.ts` and `debug-leaf-types.ts` are gone with it. They existed
so the library compiler and the debug table could share one walk; with
the library compiler no longer walking, there is one caller again, and a
shared module whose stated reason is false is worse than the duplication
it was preventing.

AN ARCHIVE WITHOUT LOCALS NO LONGER FAILS THE BUILD
---------------------------------------------------
It warns. Refusing would strand anyone using a third-party .stlib they
have no way to rebuild — a library installed from the catalogue is not
something the user can recompile. Retain covers the visible surface, and
the warning names the block and says what is missing, so a partial
retain is never silent.

A GAP THIS FOUND
----------------
Library struct types were registered with a self-referential
`TypeReference` as their AST definition, which the debug walk reads as
"opaque, do not descend". Harmless while those types were only
type-checked; wrong the moment a retained library block held one, since
the struct's fields never reached the blob and the instance restored
around a hole. Structs that export their fields now get a real
`StructDefinition`. Caught by a new test that compiles a library the way
a user would and retains an instance of its block — through a mangled
member, a library-internal struct, and a nested FB instance.

Output is unchanged where it matters: the same project produces the same
14 leaves, the same 54-byte blob and the same layout hash 618b1d38 as
the flattened design, and the SLM-RP4 restores identically — a retained
TON back at Q=True ET=10s STATE=2 two seconds after reload while its
un-retained twin restarts at ET=3s260ms.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_012FNp61926UEggtmsQPj3A3
…eclarative-locals

refactor(retain): describe library FB locals, don't pre-flatten them [NODE-94 phase 6]

@dcoutinho1328 dcoutinho1328 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff against development (docs, frontend lexer/parser/AST, semantic analyzer, backend codegen/debug-table-gen, the runtime C++ headers, and the library compiler/loader/manifest), plus the new and updated tests. The feature itself — leaf-addressed retain table, the blob format, CONSTANT's read-only gate, and retaining a library FB's internal state — is well designed and heavily tested, including real g++-compiled round-trips. One memory-safety issue in the shared runtime blocks, the rest are small cleanups. Full breakdown in the summary comment.

Comment thread src/runtime/include/iec_retain.hpp Outdated
if (blob[2] != FORMAT_VERSION) return LoadResult::BadFormat;

const uint16_t payload = get_u16(blob + 8);
if (len < static_cast<size_t>(HEADER_SIZE) + payload) return LoadResult::Truncated;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness / memory safety — integer overflow bypasses the truncation check on 16-bit size_t targets (AVR)

len < static_cast<size_t>(HEADER_SIZE) + payload can wrap on any platform where size_t is 16 bits — which is exactly avr-gcc's size_t (unsigned int), the target this header is explicitly vendored into per the file's own docstring ("the Arduino firmware ... vendor this directory").

payload (line 271) comes straight from 2 untrusted bytes inside the stored blob. If a corrupted or crafted blob sets payload close to 65535, HEADER_SIZE (14) + payload wraps mod 65536 to a small value, the truncation check on this line passes even though the real buffer (len) is far smaller, and the very next lines call crc32(blob + HEADER_SIZE, payload, crc) — reading up to ~64 KB past the end of blob. That defeats this function's own stated contract ("a corrupt or stale store degrades to a cold start rather than to plausible-looking garbage") on precisely the platform the contract exists for — a chip with far less than 64 KB of RAM to begin with.

Fix: avoid the overflowing addition — subtract instead, now that len >= HEADER_SIZE is already established a few lines up:

if (len - HEADER_SIZE < payload) return LoadResult::Truncated;

pack()'s mirrored total computation (line 216) isn't reachable this way: payload_size() there reflects this program's own compiled retain set, not untrusted storage — this is specifically an unpack()-on-corrupted-input problem.

Comment thread src/backend/debug-table-gen.ts Outdated
/** Mirrors LEAF_FLAG_RETAIN in runtime/include/debug_table.hpp. */
export const LEAF_FLAG_RETAIN = 1 << 1;

/**

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Doc comment orphaned from its function

This JSDoc block ("Render an entry's flags byte as C++") documents flagsLiteral, but flagsLiteral is now defined 60+ lines below (around line 144). What sits directly under this comment is applyBlockFlags, which already has its own correct doc comment immediately below this one (lines 89-97). A reader scanning top-to-bottom sees two JSDoc blocks stacked above applyBlockFlags and no doc at all above flagsLiteral.

Looks like a reordering left this behind. Move it to sit directly above flagsLiteral.

return flags;
}

/**

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Same issue — doc comment orphaned from its function

This block ("FNV-1a (32-bit) over the retain layout...") documents retainLayoutHashOf, but that function is defined further below (around line 129). Directly under this comment sits another, correctly-placed doc comment for RETAIN_HEADER_SIZE (lines 122-126) followed by the constant itself. Same fix as the other comment on this file: move this block down to sit directly above retainLayoutHashOf.

Comment thread src/backend/debug-table-gen.ts Outdated
path,
size,
})),
retainLayoutHash: retainLayoutHashOf(retainVars),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Simplification — same hash computed twice

retainLayoutHashOf(retainVars) is called here and again at line 926 for renderCpp's parameter, over the same retainVars array both times. For a project with many retained leaves (the bundled libraries alone list thousands) this redoes the whole FNV-1a walk for no reason. Hoist into one local, e.g. const retainLayoutHash = retainLayoutHashOf(retainVars);, and pass that to both renderCpp(...) and the debugMap object.

Comment thread tests/backend/debug-table-gen.test.ts Outdated
});
});

describe("retain through a user-installed library", () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Nit — indentation implies the wrong nesting

describe("retain through a user-installed library", ...) is written at column 0, reading as a top-level sibling of describe("retain table", ...) (opened at line 762). It's actually nested one level inside it — describe("retain table") isn't closed until line 1193, after this block. Its sibling describe("layout hash", ...) just above (line 1050) is correctly indented 2 spaces. Harmless for the tests themselves, just indent this block (and its its) to match.

@dcoutinho1328

Copy link
Copy Markdown

PR #222 Review

Summary

Implements IEC 61131-3 CONSTANT, RETAIN, NON_RETAIN (and PERSISTENT folded into RETAIN) end to end: lexer/parser tokens, semantic validation with correct block-type and scope restrictions, a leaf-addressed retain table (replacing the old per-program offsetof-based RetainVarInfo), a LEAF_FLAG_READONLY runtime gate that refuses writes/forces to CONSTANT leaves, and retaining a library function block's internal state (not just its interface) via a new declarative locals field in the library manifest. Heavily tested, including real g++-compiled round-trips of the retain blob and the read-only gate.

Module: Frontend (lexer / parser / AST)

Checklist Results

  • NON_RETAIN / PERSISTENT tokens added correctly, before AT doesn't matter but ordering vs. existing keyword/token lists looks consistent with the rest of the file.
  • MANY2 instead of OPTION for the qualifier set is the right call given qualifiers can combine (RETAIN PERSISTENT) — grammar stays permissive, semantics does the rejecting.
  • ⚠️ (non-blocking, not filed as a fix-me) The PR description says "contradictory or repeated qualifiers are a semantic error." Contradictions are caught (RETAIN+CONSTANT, RETAIN+NON_RETAIN, CONSTANT+NON_RETAIN), but a literal repeat of the same qualifier (e.g. VAR RETAIN RETAIN x : DINT;) parses and compiles silently — harmless today since isRetain/isConstant are booleans, but it doesn't match the stated claim. Flagging as an FYI, not asking for a fix.

Module: Semantic (analyzer / symbol-table)

Checklist Results

  • hasInstanceState threading is careful about not conflating "no instance" (FUNCTION/METHOD) with the existing scopeType check used for located-variable rules — the comment on why scopeType alone can't answer this is a good catch of a subtle trap.
  • ✅ Contradiction checks ordered before block-type checks, with a clear rationale ("once two qualifiers disagree there is no single intent left to validate the rest against").
  • RETAIN scope restrictions correctly relaxed to match IEC 61131-3 / CODESYS (VAR_INPUT/VAR_OUTPUT now allowed) — and the existing test suite was updated in place to assert the new, correct behavior rather than just adding new cases, so the old (wrong) restriction can't silently come back.
  • cppName / libraryName additions to VariableSymbol / FunctionBlockSymbol are narrowly scoped and documented with the "why" (can't always re-derive mangling against the wrong compilation unit).

Module: Backend (codegen / debug-table-gen)

Checklist Results

  • ✅ Old per-program RetainVarInfo / offsetof-based table cleanly removed from codegen.ts, including the doc comment explaining why the two ProgramBase vtable slots are kept (ABI pinning across the .so boundary) rather than deleted.
  • ✅ Flags threaded as an explicit parameter down the leaf walk rather than mutable shared state — correctly reasoned given NON_RETAIN needs to clear a bit mid-subtree.
  • ✅ Good test coverage for the walk: CONSTANT propagation through structs/arrays, RETAIN inheritance into nested FB instances, NON_RETAIN opt-out without leaking to siblings, library FB locals (both bundled and user-installed archives), layout-hash stability/invalidation.

Code Improvement Findings

  • ❌ (inline comment) None here — the one blocking finding is in the runtime header, see below.
  • ⚠️ (inline comments posted) Two JSDoc blocks got orphaned from their functions during what looks like a reorder (flagsLiteral's doc now sits above applyBlockFlags; retainLayoutHashOf's doc now sits above RETAIN_HEADER_SIZE) — see inline comments for exact lines.
  • ⚠️ (inline comment posted) retainLayoutHashOf(retainVars) is computed twice for the same input (lines 926 and 945) — trivial to hoist into one local.

Module: Runtime (C++ headers)

Checklist Results

  • Entry.flags repurposing the old _pad byte is a genuinely nice ABI-neutral way to add the gate — sizeof(Entry) is unchanged on every target, called out explicitly in the comment.
  • ✅ Both AVR read_entry branches (far/near) were updated to read the new flags byte at the right offset; the non-AVR branch already gets it for free via the struct copy. Verified the offset math (sizeof(void*) + 1) against the actual AVR struct layout (4 bytes: 2-byte ptr, 1-byte tag, 1-byte flags) — correct.
  • handle_set / handle_write both gate on LEAF_FLAG_READONLY before doing anything else, and the "why refuse unforce too" reasoning (an operation that never happened can't be reported as cleared) is sound.
  • pack() / unpack() blob format: header layout, CRC coverage (header minus the CRC field itself, then payload), and "order is the packing order" addressing are all internally consistent, and pack()'s own bounds check (cap < total) is safe since payload_size() there reflects this program's own compiled, build-time-bounded retain set.
  • ❌ (inline comment posted) unpack()'s truncation check (len < HEADER_SIZE + payload) can integer-overflow on 16-bit-size_t platforms — i.e. AVR, the exact firmware target this header is vendored into — because payload is 2 untrusted bytes read straight from the stored blob. A corrupted/crafted blob can wrap the check and cause the following crc32() call to read up to ~64 KB past the real buffer. See the inline comment on src/runtime/include/iec_retain.hpp:272 for the concrete scenario and a one-line fix (subtract instead of add, since len >= HEADER_SIZE is already established above).

Module: Library (compiler / loader / manifest)

Checklist Results

  • locals on LibraryFBEntry is declarative (field descriptions, not pre-flattened leaves) — reuses the exact same walk the debug table already uses for user-defined FBs, so one walk covers both, and it degrades gracefully (with a named warning) for archives built before the field existed.
  • libraryName marker replacing the "flat arrays populated" proxy for "is this FB a library one" is a real correctness fix in its own right — the proxy failed quietly in both directions per the comment, and this closes that.
  • buildLibraryTypeDefinition giving library structs with exported fields a real StructDefinition (instead of the historical self-referential alias) is exactly what's needed for the debug/retain walk to descend into them, and is covered by the "user-installed library" test exercising a library-internal struct type.

Cross-Module Observations

  • The PR description references a leaf-walker.ts as the shared walk between the debug table and the library compiler; no such file exists in the diff or the repo — the shared walk logic actually lives inline in debug-table-gen.ts's visitTypeRef/memberCppName. Purely a description inaccuracy, no code impact — not asking for a change, just flagging so it doesn't confuse someone searching for the file later.
  • Test coverage is unusually strong for this kind of change: real g++-compiled round-trips for both the CONSTANT read-only gate and the retain blob pack/unpack cycle, not just AST/manifest assertions.

Verdict

  • Request Changes
  • Blocking: the unpack() integer-overflow / out-of-bounds-read on 16-bit-size_t targets (src/runtime/include/iec_retain.hpp:272). Everything else is a small, optional cleanup.

…-table-gen

Review findings from PR #222:

- unpack() computed `len < HEADER_SIZE + payload` with payload read straight
  from the (untrusted, possibly corrupted) blob header. On a 16-bit size_t
  target (avr-gcc, the firmware this header is vendored into) that addition
  can wrap, letting a corrupted payload_len bypass the truncation check and
  send crc32() reading tens of KB past the real buffer. Compare via
  subtraction instead, now that len >= HEADER_SIZE is already established.
- Two JSDoc blocks in debug-table-gen.ts had drifted off the functions they
  documented (flagsLiteral's doc sat above applyBlockFlags;
  retainLayoutHashOf's sat above the unrelated RETAIN_HEADER_SIZE constant).
  Moved each back above its own function.
- retainLayoutHashOf(retainVars) was computed twice for the same input;
  hoisted into one local.
- Fixed indentation in debug-table-gen.test.ts where a describe block was
  nested one level deeper than its indentation implied.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01D7txRxvUEhLirT4PDNM6Pf
@thiagoralves

Copy link
Copy Markdown
Contributor Author

Review response — all five findings were already fixed

@dcoutinho1328 you fixed all of these yourself in 4197860, so there is nothing from me on this branch. I verified each rather than take it on trust:

Finding State
iec_retain.hpp:272 — 16-bit size_t overflow Fixed — len - HEADER_SIZE < payload
debug-table-gen.ts:80 — orphaned flagsLiteral doc Fixed — doc now adjacent
debug-table-gen.ts:107 — orphaned retainLayoutHashOf doc Fixed — doc now adjacent
debug-table-gen.ts:945 — hash computed twice Fixed — one call site remains
test file — describe at column 0 Fixed

On the overflow, because it deserves recording

Your analysis was right and the consequence is worse than "the check is wrong". I reproduced the arithmetic:

/* AVR: size_t and unsigned int are both 16-bit */
14 + 65530 = 8len(100) < 8 is FALSE → check PASSEScrc32(blob + 14, 65530, crc) reads 65530 bytes
                            off a 512-byte buffer

/* 32/64-bit host */
14 + 65530 = 65544len(100) < 65544 is TRUE → Truncated (safe)

So it was an out-of-bounds read of up to ~64 KB, reachable from two untrusted bytes in the stored blob, on a chip with a few KB of RAM — and unreachable on the Linux runtime, which is the one host where it would have been harmless anyway. Exactly the platform the "degrades to a cold start" contract exists for.

Worth a regression test for the Truncated path with a payload field near 0xFFFF, since the round-trip suite currently only exercises a short blob (HEADER_SIZE - 1) and would not have caught this. Happy to add it here if you want it in this PR.

Still blocking downstream, and not a code change

openplc-editor and openplc-web read debugMap.retainBlobSize, which exists only on this branch. Their binary-versions.json pins v0.6.3, and neither it nor v0.6.4 carries retainBlobSize or LEAF_FLAG_RETAIN — so editor #1034 / web #691 compile locally only because a strucpp build is rsynced into node_modules.

This PR needs to merge and release, and the pin bumped, before those two can go. Flagged in their PR bodies too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants