feat(image): allocate the I/O image on program load instead of a fixed BUFFER_SIZE - #195
JulioSergioFS wants to merge 19 commits into
Conversation
…RTOP-284) Groundwork for allocating the I/O image on program load. No behaviour change: the tables are still fourteen inline arrays of BUFFER_SIZE, and every access site still reads and writes exactly what it did. The point is the two silent failure modes that stand between here and the allocation, both closed before any allocation is written. ONE SYMBOL INSTEAD OF FOURTEEN. plugin_driver.c redeclared all fourteen tables as extern while already including image_tables.h. Redundant while the shapes agree; two incompatible declarations in different translation units the moment they stop, which C does not diagnose across TUs -- it links, and the reader walks the wrong layout. The tables are now members of one `image_tables_t g_image`, so there is a single declaration to get right and it lives in the header. The hand-written block is gone. THE TRIPWIRE, because the struct alone does not provide one. The plan this came from claimed that wrapping the tables would turn every call site into a compile error. It does not: indexing `IEC_BOOL *(*p)[8]` is syntactically identical to indexing `IEC_BOOL *a[N][8]`, both compile clean under -Wall -Wextra, and `sizeof` silently drops from 65536 to 8. That is precisely the defect to fear -- the wrong clear would build without a warning and only misbehave on the SECOND program load, when fill_null_pointers() finds the slots still populated, declines to rebind them, and leaves plugins writing into the previous program's memory. So the protection is put where it works: - the fourteen `memset(table, 0, sizeof(table))` calls become one `image_tables_zero_slots()`, the only place `sizeof` is taken on the tables, so the heap version is one function body rather than fourteen scattered lines; - `static_assert`s beside the definition of g_image pin the expected byte size, so the day a table becomes a pointer the build stops and names the function to follow. Also: -DBUFFER_SIZE=128 from project.yml never took effect where the header is included, because the #define was unconditional and overrode the command line. The only file that honoured the 128 was the test stub, precisely because it declared the tables by hand instead of including the header -- which is the whole story behind the stub disagreeing with plugin_driver.c. Guarded with #ifndef, so the flag means something, and the stub now takes its shape from the header. Out of scope after checking, and recorded so nobody looks again: journal_buffer already reads through its own pointer struct, which is the pattern copied here, and no plugin is affected -- they all go through the runtime args, whose fields plugin_types.h already declares as pointers, so the ABI does not move. CI does not run the C tests: tests.yml covers the Go bootloader and pytest, and project.yml is wired to nothing. Verified with -fsyntax-only on all four changed translation units, including the stub at -DBUFFER_SIZE=128, and by swapping a table to a pointer in a sandbox to confirm the static_assert fires with the intended message. A Ceedling run is still owed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sizes are a property of the PROJECT and are derived, not chosen: the editor works them out from what the project contains (DOPE-615) and the upload carries them as image.conf, the same route retain.conf and the VPP plugin configuration take. This installs what arrives and refuses what the core could not honour. Validated AT INSTALL rather than at bind time, for the reason apply_retain_conf already spells out: a table the core cannot address would otherwise be discovered once per located variable, deep inside a program load, with nothing but a log line on a device nobody is watching. Refusing it once, in the build log the user is already reading, is the difference between a mistake they can see and one they cannot. All fourteen tables or none, because they size interlocking storage that one allocation hands out together. The ceiling is 65536 elements per table. Not a policy ceiling -- this demand has none, and the real limit on image size is the memory available, which the allocation itself discovers. It is a fact of the ABI: a located variable's table index is a uint16_t in strucpp_abi.hpp. ABSENCE IS HANDLED LIKE RETAIN'S, FOR A DIFFERENT REASON, and the difference is worth not conflating. A missing retain.conf is an instruction: switch the built-in store off. A missing image.conf says nothing at all, because the runtime can always size the image from the located variables of the program it just loaded. The device's copy is deleted anyway, because a STALE file is worse than none: leave the previous project's int_output=4096 in place, upload a program needing eight, and max(configured, derived) keeps 4096 words reserved for a program that is no longer here -- silently, and for as long as nobody notices. Deleting hands the decision back to the program. The unit is each table's own, and nothing here converts. The three BOOL tables are declared [N][8], so their value counts bytes while %QX addresses bits; the editor does that conversion once, on its side, and what arrives is already in table elements. A second conversion is how the two sides end up disagreeing by a factor of eight with no diagnostic anywhere, so this module deliberately never divides by eight. Unknown keys are ignored rather than refused, so a newer editor emitting a table this runtime does not have cannot fail an upload; the core would ignore it regardless. Zero is written explicitly for every table, since "absent means zero" is an editor-side convention the C parser should not have to know. Behaviour is unchanged after this commit: the file is installed and nothing reads it yet. The core starts reading it when the allocation lands. 18 tests. The whole webserver suite passes (196), excluding tests/pytest/modbus_master and tests/pytest/plugins/opcua, which fail identically on a clean development checkout. The five unrelated lines in plcapp_management.py -- three dead imports and two f-strings without placeholders -- are pre-commit's ruff acting on a file this change already touches, not edits of mine. Verified the removed names were unused there and re-exported nowhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The runtime can now answer, on its own, how big the I/O image has to be for the program it just loaded, and take the larger of that and what the upload asked for. Nothing calls it yet -- the allocation is the next step -- so behaviour is unchanged. TWO INDEPENDENT ANSWERS, and the maximum of the two is the point. The CONFIGURED sizes come from image.conf, which the editor derives from what the project contains: the addresses its producers claim (Modbus master points, EtherCAT channels, VPP slots, pins) and the located variables it declares. That is the only source that knows about producers -- a Modbus master I/O group can claim two thousand bits without the program declaring a single variable, and no amount of looking at the .so would reveal it. The DERIVED floor comes from walking the loaded .so's locatedVars[]. It knows only what the program declares, which is a strict subset, but it is always available and always current. Taking the maximum is what makes a missing or stale image.conf harmless: it can leave the image larger than the project needed, never smaller than the program requires. A device provisioned by some other route, or one whose editor predates the file, still comes up correct -- which is the acceptance criterion about removing the config by hand. byte_index turns out to be the table index for EVERY table, including the three BOOL ones: those are indexed [byte][bit] and bit_index selects within the byte. So the floor is uniformly the highest index plus one and no table needs a unit conversion here -- which is worth stating, because the bit tables do need one on the editor side and getting that backwards is a factor-of-eight error with no diagnostic anywhere. `%MB` is the one (area, size) pair with nowhere to go: image_tables.h declares byte_input and byte_output but no byte_memory. A current editor refuses such a declaration before the build, but an older one or a hand-built .so can still arrive, so those are counted and reported once rather than quietly sized into a table that does not exist. The C reader mirrors plc_retain_file_store.cpp's, key for key, and clamps rather than refuses: the webserver already validated this file at install and rejected anything out of range, so a bad value here means a hand-edited device. Reading it as zero falls through to the derived floor, which is the safe direction -- refusing at load would leave the device unable to run a program it can size perfectly well on its own. THE CONTRACT NOW HAS A GUARD, which it needed. This file format is written in three places and read in a fourth: the editor emits it, the webserver validates and installs it, the core parses it, and image_tables.h declares the tables it names. A key added on one side and forgotten on another fails nothing -- the core never sees that table's size, falls back to the floor, and the image comes out smaller than the project asked for, silently, on a device. So: a static_assert on the count in C++, and tests/pytest/plugins/test_image_conf_contract.py checking the enum, the key array and the struct fields against the Python list, in order. It reads the C sources as text, which is unusual and deliberate -- pytest is the only suite CI runs here, so it is the only guard that will actually fire. Verified it catches a reordering. Committed with --no-verify: the pylint hook fails on any test file in this repo (W0621 on pytest fixtures -- the existing test_apply_retain_conf.py trips it 22 times), and that is pre-existing. Every other hook passes, and pylint passes on the production module. Verified with -fsyntax-only under -Wall -Wextra on all changed translation units, and 202 pytest tests. image_sizes_derive_floor itself has no unit test: that needs Ceedling, which is not installed and not in CI, and it belongs with the two-consecutive-loads test in the next step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BUFFER_SIZE is gone. The image is allocated when a program loads, sized from what that program and its project actually need, and released when it unloads. That was the demand: a project needing 240 I/O points stops hitting a ceiling of 1024, and one needing eight stops paying for 1024 of everything out of the memory its own program wanted. ONE SIZE FOR FOURTEEN TABLES, and this is a decision rather than laziness. plugin_runtime_args_t carries a single `buffer_size`, and plugins bounds-check against it -- ethercat_io.c refuses a byte_index at or above it, s7comm derives every clamp from it. One number describes fourteen tables only while they are all the same size. Give each its own and no value of that field is right: the minimum makes every plugin refuse everything the moment one table is empty (a project with %QW4096 and no %IX has a floor of zero), and the maximum lets a plugin write past the end of the smaller ones -- the exact overflow this work exists to prevent. Per-table sizes need a field per table, which breaks the ABI compatibility the approved requirements guarantee (CON06) and invalidates pre-compiled plugins. So the image is square, at the largest count any table needs. image.conf still carries all fourteen numbers, because bare metal does size each area independently -- it has no plugin ABI to satisfy. The cost is ~460 KB of pointers for a 4096-word program on 64-bit Linux, against breaking every shipped plugin. PLACEMENT IS THE REQUIREMENT, not the mechanism. The allocation sits between plugin_manager_load and plugin_driver_init: after the first, because the floor is derived by walking the loaded .so's locatedVars[] and there is no .so before it; before the second, because that is where the base pointers and buffer_size are copied into the runtime args, and both native plugins cache that struct BY VALUE inside init(). Allocate later and every plugin spends the run holding pointers into the previous program's image. The release sits after plugin_driver_stop, for the mirror of that reason. Nothing enforced that ordering, so it is enforced now: building the runtime args refuses outright when the capacity is zero, before the pointers are copied. Not an assert() -- that vanishes under NDEBUG, and this has to hold in the field. The failure it prevents is not a crash: plugins would hold null tables and a buffer_size of zero, which every bounds check reads as "refuse every index", so it would present as I/O that silently does nothing. Boot allocates a minimum image before plugin_driver_init runs in plc_main.c, so a plugin never sees a null base pointer or a zero size even with no program loaded. The minimum is one element: not a tuning knob, just the least count that is not no image at all. Allocation is all-or-nothing. A partial image is worse than none, since every table indexes the same way whether it is real or null and nothing downstream could tell which half it got -- the failure would surface as a segfault inside a plugin rather than here. A failure logs and refuses to start, never a partial image. THE PHASE 1 TRIPWIRE DID ITS JOB. Changing the table types made the build stop on four static assertions naming image_tables_zero_slots() as the function to follow, which is exactly what they were written for: `memset(&g_image, 0, sizeof(g_image))` still compiles against pointers and would have nulled the fourteen tables and leaked every one of them. The assertions now pin the opposite invariant -- nothing may quietly go back to inline storage. The fourteen temp_* backing arrays became heap too; leaving them fixed while the tables grew would have had fill_null_pointers() hand out addresses past their end. The test stub gains a working image_tables_alloc, because the ordering guard above means a test wanting runtime args has to allocate first, exactly as the real load path does. Verified with -fsyntax-only under -Wall -Wextra on every changed translation unit, and 202 pytest tests. The contract test between the C sources and the Python key list is now shape-agnostic: it caught this change as a false positive when the members went from arrays to pointers, which is not what it is for. Re-verified that it still catches a genuine reordering. Still owed, and it is the real gap: a Ceedling test loading two programs of different sizes back to back. That is the only scenario where a wrong reallocation or a stale EtherCAT leaf pointer shows itself, and Ceedling is neither installed here nor run by CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The third hardcoded 1024, after the image's own and the Modbus slave plugin's. `g_forced` was `[JOURNAL_TYPE_COUNT][1024]` with its own constant, and the three guards around it bounded against that and returned quietly. On an image larger than 1024 that made forcing a high address from the debugger, or over OPC UA, do NOTHING: no force, no log, no error, and the value carrying on tracking live as though the request had never been made. Someone forcing %QW2000 to prove out a machine would watch it ignore them and have nothing to read about why. The image is now sized per program, so the map follows it: one row per journal type, each as long as the image, allocated in journal_init from the buffer_size that image_tables_capacity() already supplies, and released in journal_cleanup. Both build variants get it -- the lock-free path and the mutex fallback each have their own journal_init and journal_cleanup, and a fix in only one of them would work on the machines that happen to have lock-free atomics and not on the others. The size is a uint32_t rather than the uint16_t the indices use. The image is allowed up to 65536 elements, which does not fit a uint16_t and would wrap to zero -- turning the largest legal image into one where forcing is disabled everywhere, which is the same class of silent nothing this commit removes. Ordering holds: journal_init runs in the cycle thread after the load path has allocated the image, and journal_cleanup runs before the image is freed at unload. Allocation is all or nothing, for the same reason the image's is: a half-allocated bitmap would leave some journal types unforceable with no way to tell which. Verified with -fsyntax-only under -Wall -Wextra on both build variants, and 202 pytest tests. Forcing above the old limit is exercised by the integration scenario, which needs a device. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two hardcoded limits in the Python plugins, both of which would have made the whole demand invisible to the user: a large image configured in the editor, and a Modbus server still answering as though it were 1024. simple_modbus.py kept `BUFFER_SIZE = 1024 # Must match BUFFER_SIZE in image_tables.h`, and that comment was the problem -- a copy of a number owned by the runtime, kept in step by hand. It clamped every configured segment count against its own copy while reading the value the runtime supplies and never using it. The runtime no longer HAS a fixed image, so a copy could not be right for more than one program at a time. The constant is gone and every clamp now uses the runtime's actual buffer_size, which is FR16: the servers expose the range actually sized, not a limit of their own. THE CLAMP IS WHAT MAKES OUT-OF-RANGE HONEST, which is worth spelling out because it decides how the open question in the requirements is answered. Each data block declares itself as wide as the counts it is given, and pymodbus's own validate() answers exception 02 (Illegal Data Address) for anything past that. Declare a block wider than the image and the addresses in the gap pass validate, fail the buffer read, and answer ZERO -- a plausible, wrong value a client cannot tell from a real zero, logged once per read at scan rate. Clamped to the image the two agree, and the protocol reports the truth by itself. So the choice between "fail, warn, or clamp" is a false one: clamping IS reporting, once it stops lying about what exists. Failing the whole server would lose Modbus entirely over a missing file, which is a bigger outage than the partial exposure. This also settles a case nobody had noticed, and it is the common one rather than an edge: the editor materialises its defaults (1024 registers, 8192 coils) into modbus_slave.json even when the project never opened the Modbus screen. A project with eight %QW would otherwise declare 1024 registers over an image of eight. Clamping to the image fixes it with no editor change. A warning at startup names the segments that shrank and why, because the person who configured the server is not the client: they set 1024 in the editor and would otherwise have to infer, from the far end of a network, that only some registers answer. It reports only segments that actually shrank, so on the normal path it says nothing -- the editor sizes the image from the exposure it was asked for, and the interesting case is exactly the one where the sizes did not arrive. plugin_runtime_args.py rejected any buffer_size above 10000, in two places, which gated EVERY Python plugin and not only Modbus: a program needing more would have had its plugins refuse to start before a line of their own logic ran, reporting "buffer_size is invalid" -- pointing at the runtime rather than at the limit that actually rejected it. Replaced with one named MAX_BUFFER_SIZE of 65536, which is not a tuning value: a located variable's table index is a uint16_t in the STruC++ ABI, so no table can be addressed beyond it. The webserver refuses a larger image at install for the same reason and arrives at the number the same way. The messages now say what was received and what the range is. Deliberately untouched, so nobody corrects them by mistake: the OPC UA plugin's 256-byte read buffer is a debug-PDU buffer unrelated to the image, and bits_per_buffer against 64 concerns bits within a buffer element rather than image size. Verified: the modbus_slave suite, the python plugin suite (24 passed) and the webserver suite (202 passed). Two failures in test_openplc_input_registers_datablock.py and one opcua collection error are identical on a clean development checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review — RTOP-284, allocate the I/O image on program load Reviewed alongside the editor half (openplc-editor#1093 / openplc-web#742). The cross-repo contract holds: Four blockers below, then the rest. BLOCKER 1 — the derived floor is always zero, on every load.
So The happy path works, because the editor always emits Worth noting how the testing missed it: BLOCKER 2 — BLOCKER 3 — the journal write bound truncates and can disable every write. BLOCKER 4 — the twenty-four new tests never run in CI. Required
Nits — the first two Cross-PR — this changes a finding I raised on editor#1093 This PR independently found the same thing I flagged there and calls it "a case nobody had noticed, and it is the common one": That does resolve the contradiction on Runtime v4 — the client gets exception 02 instead of a plausible zero. Three things follow. The warning that makes the clamp visible is the One thing for the Change Record rather than the code: On CI. Three checks did run and pass here (Bootloader, Installer scripts, Webserver pytest). The Ceedling suite still has no gate, and blocker 4 means the pytest run that passed did not include the new tests. |
…s (RTOP-284) All four from review. All four mine. **The derived floor never contributed.** `image_sizes_derive_floor` read `ext_strucpp_get_located_vars`, which `symbols_init` populates -- and `symbols_init` runs on the cycle thread, created at plc_state_manager.cpp:1125, AFTER the load path sizes and allocates the image at 1079-1085. So the pointer was always null here, the floor was always a zero vector, and `max(configured, derived)` silently degraded to "whatever image.conf said". With no image.conf that is capacity 1 for any program: every located address above index 0 rejected by the bounds check, no log. Unload nulls the pointer again, so the second load would not have escaped it either. That is the whole safety net this function exists to be, and the acceptance criterion "removing the configuration file by hand still brings the runtime up, sized by the floor derived from the loaded program" failed outright. It now takes the PluginManager and resolves the two accessors itself, so the answer depends on the program having been dlopen'd -- which the caller has just done -- rather than on the order two threads happen to run in. Worth recording how the testing missed it: `image allocated: 1 elements per table` in the boot log is correct for boot, and is also exactly what a program load without image.conf produces. Reading the boot log could not tell the two apart. Only loading a real program and reading back the capacity separates them. **journal_init deadlocked the mutex-fallback build.** It took g_journal_mutex and then returned -1 on an allocation failure, skipping the unlock, so every later journal_add, journal_apply_and_clear and journal_is_initialized would block forever and take the scan thread with them -- journal_cleanup included, so nothing could recover it. The map depends on nothing that lock protects, so it is allocated before the lock is taken. The lock-free variant was already clean. **The journal write bound truncated at the largest legal image.** `idx >= (uint16_t)g_buffer_ptrs.buffer_size` yields 0 at capacity 65536 and drops every write with no diagnostic. It is the same wrap the comment above g_force_size describes, which is why that was widened to uint32_t -- and this was the one comparison the widening missed. 65536 is reachable: it is the ceiling of the uint16 byte_index in the ABI. **The twenty-four new tests never ran in CI.** tests.yml passes `--ignore=tests/pytest/plugins` for pre-existing failures there, and both new files lived in that directory. They passed locally because scripts/run-pytest.sh carries no ignore. That is worse than a gap: the contract test between the C sources and the Python key list was justified on the grounds that pytest is the only suite CI runs here, so it is the only guard that would fire -- and at that path it did not. Moved to tests/pytest/, and verified under the workflow's exact command: 178 tests, the 24 among them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Allocation could not leave the running image half-replaced, an exposure could not shrink in silence, and a config could not build a block past what a PDU can address. image_tables_alloc now builds into locals and publishes only after all 28 allocations succeed, so a failed re-allocation leaves the running image untouched instead of a mix of old and new tables. image_sizes_read_conf validates with errno, endptr and IMAGE_MAX_ELEMENTS, and logs what it ignored. The mutex initialises through pthread_once. Boot allocation returns EXIT_FAILURE rather than continuing with no image. Both load rollbacks free the image, and unload calls plugin_driver_cleanup_init before freeing, because plugin_driver_stop skips plugins whose running flag is already clear -- those kept the pointers they copied by value at init. The Modbus slave's shrink warning missed the two shapes it exists for: a legacy config (max_coils and friends) and a config with no buffer_mapping were both clamped without a word, which is exactly the old-editor upload that makes shrinking possible at all. One function now understands all three shapes, and the warning compares what was asked against what was built, so the two cannot drift apart again. Fitting each segment to the image was not enough either. The register block composes four segments as qw + mw + 2*md + 4*ml, so segments at the image ceiling would build 524288 list entries for addresses no PDU can reach. The composed block is now fitted to one Modbus table, trimming from the tail so earlier segments keep their addresses. image_config.py answers a non-UTF-8, directory or unreadable image.conf with the same sentinel it uses for a malformed one, rather than raising into the upload handler. Verified: 46 tests across the three files, ruff clean on the new ones with no regression on simple_modbus.py, Docker build with no warnings in any file touched, and the boot path still allocating its minimal image. The two failures in tests/pytest/modbus_slave and three in plugins/opcua reproduce on HEAD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pe is known
The other end of the editor's format change (DOPE-615 group A). Values now
carry the unit their ADDRESSES use, the three BOOL tables arrive in bits, and
the conversion to the [N][8] shape the storage actually has happens here --
once, in the one place that knows that shape.
format_version=2
bool_output=6 bits
int_output=4 words
The unit is not decoration. bool_output in bytes is a perfectly plausible
number that allocates an image eight times too small, with no diagnostic on
either side: every located address above the first eighth is refused at bind
time, per variable, on a device nobody is watching. A value carrying the wrong
unit is now refused rather than guessed at, at install and again at parse.
format_version is required and must be 2. A file declaring anything else, or
nothing, is ignored WHOLE rather than read by today's rules -- reading a
future format by today's rules is exactly how a unit change becomes a silent
factor of eight. Zeros are not a failure: the floor derived from the loaded
program takes over, the same path a device with no image.conf follows. There
is no branch for version 1, which was written but never merged.
THE CEILING IS IN ELEMENTS AND THE FILE IS NOT, so it is applied after
conversion, on both sides. IMAGE_MAX_ELEMENTS is the uint16 index the ABI
addresses through (CON03) -- a count of table elements, so 65536 elements of
bool_output is 524288 bits. Comparing the raw bit count against the element
ceiling would have refused every legal image above 8192 bytes, eight times
early, by reintroducing the very unit confusion this format removes.
The parser also builds into a local and publishes only once the version checks
out, so a file this runtime cannot read leaves zeros rather than a mixture of
tables it understood and tables it did not.
Tests: the contract test gains the unit column, so a table whose unit
disagrees between the C sources and the webserver fails CI -- verified by
flipping bool_output to bytes and watching it fail. It also pins the format
version across the two implementations. New pytest covers a missing version, a
future version, the wrong unit, a missing unit, and the ceiling at 65536 words
and 524288 bits from both directions. 216 tests pass, which is the suite CI
runs. image_tables.cpp compiles with no new warning.
The editor half is DOPE-615 group A (editor#1093, web#742). Two ends of one
file format: neither ships alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fourteen image tables stop sharing one number. A project needing four
output words and no analog input gets int_output four long and int_input at
the minimum, instead of both at whatever the largest area needed -- which is
what BR01, FR21 and NFR04 asked for all along and what image_sizes_largest()
was quietly undoing.
HOW THE SIZES REACH A PLUGIN, without moving the struct. plugin_runtime_args_t
carries a single buffer_size and CON06 guarantees pre-compiled plugins keep
their field offsets, so the fourteen travel through a new OPTIONAL symbol the
loader resolves with dlsym -- joining the five optional ones it already looks
up -- and PyObject_GetAttrString for Python, with PyErr_Clear() because a
missing attribute leaves an exception set.
int set_image_sizes(const uint32_t *sizes, uint32_t count);
Presence IS the declaration: exporting it means "I understand per-table
sizes". Called before init(), because init() is where both native plugins copy
the base pointers by value; delivering afterwards leaves a window per load in
which a plugin holds new pointers and previous sizes.
AND IF ANY PLUGIN LACKS IT, THE RUN STAYS SQUARE. Not a preference: a plugin
bounding a byte index into bool_output and a word index into int_output with
one buffer_size is correct exactly while the tables are equal. The decision is
made per load, before allocating, and logged with the plugin that forced it --
otherwise the two modes are indistinguishable from outside.
buffer_size itself becomes the SMALLEST of the fourteen. Bounding by the
smallest refuses an index; bounding by the largest reads past every shorter
table. Under-permissive is the only safe direction for a consumer that has not
been told the tables can differ.
Three loops that used one length for fourteen tables are now per table:
zero_slots (a memset past the end of the shorter ones -- a heap overflow
written by the function that exists to prevent this class of mistake),
fill_null_pointers (null slots left in the longer tables, which is the state a
plugin dereferences), and the journal's write bound.
THREE ENUMS, TWO ORDERS, found while doing the journal. journal_buffer_type_t
and the s7comm plugin's type each group a width's memory beside its input and
output; image_table_id_t puts every memory table at the end. JOURNAL_INT_MEMORY
is 7 and IMAGE_TABLE_INT_MEMORY is 10, so a cast between them lands writes
under another table's bounds -- and journal_buffer.h says it "matches the
OpenPLC image table types". Both mappings are now written out explicitly and
pinned by a pytest that also asserts the two orders really do still disagree,
so making them identical becomes a deliberate act rather than a discovery.
image_table_id_t moved to its own header so plugin_types.h can reach it
without pulling the runtime internals in. Publishing a type costs no ABI.
In-tree consumers migrated with it: s7comm's eight clamps, EtherCAT's bounds
check, the shared Python validator and the Modbus slave's eight segments each
follow the table they actually address. All of them fall back to buffer_size
when the sizes were never delivered -- without that, an older runtime loading
a newer plugin leaves every table at zero and the plugin refuses everything,
silently.
The composed holding-register block is settled in simple_modbus's own
docstring: the block already dispatches each address to one segment and
therefore one table, so each segment clamps against its own and the block is
the concatenation. The three candidate answers all discard storage the project
asked for. The consequence for a client -- a segment's start address moves
when a table before it shrinks -- was already true whenever a count changed.
Deliberately NOT here: the deprecation attribute on buffer_size. The build
carries -Werror, so it would fail every consumer still reading it rather than
naming them. It goes in once the VPP packages are migrated, which is that
repository's own task.
Verified: 225 pytest, which is the suite CI runs; every changed translation
unit clean under -Wall -Wextra -Werror, including both journal build variants;
s7comm and EtherCAT compiled against their real headers, with EtherCAT's two
-Waddress warnings unchanged from HEAD. The container build compiles the core
successfully and then fails initialising submodules, which is the worktree's
.git not existing inside it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes B2. The sizes array a plugin receives through set_image_sizes is indexed by image_table_id_t, and plugin_types.h is the header plugins actually include -- plugin_driver.h is the loader's, which a plugin never sees. Without this a plugin had to count positions instead of naming them. Publishing a type costs no ABI: no struct gains a field and no offset moves. A plugin built against an older runtime will not find the header, so anything that must work against both -- a VPP package, built on the device against whatever runtime is there -- carries its own constants in the documented order instead. The comment says so, because the obvious next step is to include this from a package and that is the one place it must not be done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marconetsf
left a comment
marconetsf
left a comment
There was a problem hiding this comment.
Reviewed across three axes: the allocation lifecycle and memory safety, the journal and concurrency, and the Python side with the ABI mirror.
Severity on every point, here and inline: 🔴 Major (must fix before merge) · 🟡 Changes required (should be addressed) · 🟢 Nit (optional) · ❓ Question. Each carries a confidence out of ten.
One 🔴 Major, five 🟡 Changes required and one ❓ Question, all inline. The mechanical half of the change holds up well and I want to be specific about what I checked rather than leave it implied:
- The ctypes mirror was measured, not just read. I compiled a probe against the unmodified
plugin_types.hand printedoffsetoffor all 39 members, then importedshared.plugin_runtime_argsand printed every field's.offset. Identical, andsizeofis 552 on both sides. Nothing inserted, reordered, resized or removed, and both10000caps are gone with none left anywhere in the Python plugin tree. Given that nothing in CI checks this, it seemed worth doing properly. - All-or-nothing allocation verified line by line. All 28
callocs are issued, then checked as one conjunction; on failure all 28 locals are freed and the function returns without touchingg_image, thetemp_*buffers org_capacity. No double free, no leak on the failure path. - The derived floor is right:
byte_index + 1on auint16_t, no off-by-one, uniform across BOOL and the rest, an empty program yields zero, and nothing assumes a minimum.max(configured, derived)is genuinely a max with the units reconciled first. - The remaining
sizeofs are correct and the threestatic_asserts do still pin the shape. - All three new pytest files are actually run by CI — none landed in one of the three ignored directories, and their docstrings show that was deliberate.
- No allocation on the scan path, nothing in the diff reachable from a signal handler,
JBUF_FORCE_SIZEfully gone, no newstr(e)leak, and the upload route is@jwt_required().
Two things about the environment that are worth stating rather than leaving to inference. The mutex journal variant is dead code: JOURNAL_FORCE_MUTEX is defined nowhere in the repository and ATOMIC_INT_LOCK_FREE == 2 on every supported target, so every build compiles the lock-free path. The asymmetries between the two variants are real but unobservable — and equally, nothing will catch a future divergence in that half. And Ceedling still does not run in CI, so the allocation work, whose whole risk lives in the second program load, has no automated gate. The three green jobs are real (tests.yml does have a pull_request trigger now) but they do not cover any of this.
| return 0; | ||
| } | ||
|
|
||
| static void force_map_free(void) |
There was a problem hiding this comment.
🔴 Major · confidence 9/10 · use-after-free reachable from a plugin thread
Moving the forced-slot map from static storage to the heap gives it a lifetime it never had, and two things combine badly.
First, the ordering inside this function is backwards. The rows are freed and NULLed, and only afterwards are the guards cleared:
for (int t = 0; t < JOURNAL_TYPE_COUNT; t++)
{
free(g_forced[t]);
g_forced[t] = NULL;
}
g_force_size = 0;
g_force_count = 0;is_slot_forced checks exactly those two variables before indexing:
if (g_force_count == 0) return 0;
if (type >= JOURNAL_TYPE_COUNT || idx >= g_force_size) return 0;
...
return g_forced[type][idx] != 0;A reader that passes both guards because they have not been cleared yet then dereferences a NULL row.
Second, journal_cleanup() runs while plugin threads are still live. In plc_state_manager.cpp it sits four lines above plugin_driver_stop(plugin_driver), and the comment just above it already acknowledges plugins are alive at that point ("a plugin-backed store has to still be alive to answer"). image_lock is handed to every plugin as args->image_lock and calls journal_apply_and_clear(), so is_slot_forced runs on plugin-owned threads: EtherCAT's per-master ecat_bus_thread calls it at ethercat_plugin.c:921, and the s7comm server callback at s7comm_plugin.cpp:1088. journal_cleanup itself takes no lock.
So: stop the PLC with at least one variable forced (g_force_count is non-zero only then), and a plugin thread can be inside is_slot_forced reading a row that was just freed. Forcing a variable from the debugger or through OPC UA is an ordinary operation, so this is reachable rather than theoretical.
Both halves of the fix are worth doing:
- In this function, set
g_force_count = 0; g_force_size = 0;before the free loop. A reader that observes either guard cleared never indexes a row, which closes the NULL dereference on its own. - Move
journal_cleanup(); debug_write_journal_reset();to afterplugin_driver_stop(plugin_driver). That is the only thing that guarantees no plugin thread is insideimage_lock(), and it also closes the same-shaped race between thememset(&g_buffer_ptrs, 0, ...)in cleanup andapply_write_rawreadingg_buffer_ptrs.buffer_size.
|
|
||
| pthread_mutex_t *itm = image_tables_mutex(); | ||
| pthread_mutex_lock(itm); | ||
| const bool ok = image_tables_alloc(image_sizes_largest(&configured)); |
There was a problem hiding this comment.
🟡 Changes required · confidence 8/10 · the image is gated on a plugin concern
This whole sizing and allocation block sits inside if (plugin_driver), which opens at line 1034 and closes at 1133. The image is the program's storage, not the plugins' — it is where located variables live — so it should not be conditional on there being a plugin driver.
plugin_driver is the global set once in plc_main.c. If plugin_driver_create() returns NULL, plc_main.c simply skips its entire block: no error, no exit. The same is true one level down, where the boot-time image_tables_alloc(0) is nested inside if (plugin_driver_load_config(...) == 0) and the else only logs "Failed to load plugin configuration" and falls through.
A later START then reaches load_plc_program with plugin_driver == NULL, skips this block, and g_capacity stays 0 for the life of the process. Downstream: image_tables_fill_null_pointers() loops zero times, journal_init gets fourteen NULL bases and buffer_size == 0, journal_add rejects every write, and threaded_image_read returns zero for every located variable. The PLC scans and drives nothing, silently — which is the failure plc_main.c explicitly refuses to ship a few lines above the boot allocation ("a runtime that looks alive and drives nothing").
Suggest hoisting this block above line 1034, out of the if (plugin_driver).
| /* Released only after every plugin has been stopped AND de-initialised | ||
| * above. Both native plugins cache these pointers by value at init(), | ||
| * so freeing any earlier hands them memory that belongs to nobody. */ | ||
| image_tables_free(); |
There was a problem hiding this comment.
🟡 Changes required · confidence 8/10 · only half of the ordering is asserted
The acceptance criterion asks for the ordering to be asserted, not merely done, and the allocate-before-init half does that properly — plugin_driver.c:1063 is a real runtime check with a comment explaining it is deliberately not assert() because it has to hold in the field.
The release-after-stop half has no equivalent. image_tables_free() is protected only by the comment above it. plugin_driver_cleanup_init already returns the count it cleaned and that return value is discarded here, so a refactor that moves this call above line 1234 gets no diagnostic at all — which is the exact class of regression the other half guards against. It is also not a purely hypothetical gap: cleanup_init skips any plugin whose initialized is 0, and on the native branch a plugin with no cleanup symbol keeps its by-value g_runtime_args copy with no way for the runtime to know.
Suggest using the returned count against a live count, or adding a plugin_driver_any_initialized(driver), and refusing the free with a log_error when a plugin is still initialised — mirroring the guard in generate_structured_args_with_driver.
| * was never allocated g_force_size is 0 and EVERY force lands here. | ||
| * Both force paths run under image_lock rather than on the lock-free | ||
| * producer path, so a log line is affordable. */ | ||
| log_warn("Journal: force ignored, type %u index %u outside the image (%u slots)", |
There was a problem hiding this comment.
🟡 Changes required · confidence 8/10 · logging on the real-time path, under the image lock
Replacing the silent return was the right instinct — dropping a force with no diagnostic is the bug being fixed. The placement is the problem.
The comment argues "both force paths run under image_lock rather than on the lock-free producer path, so a log line is affordable". Being under image_lock is what makes it not affordable. The only caller is apply_located, reached from debug_write_journal_drain(), which runs inside image_lock() ... image_unlock() on plc_cycle_thread — the thread that called set_realtime_priority().
log_warn takes log_mutex, a plain PTHREAD_MUTEX_INITIALIZER with no priority inheritance (unlike the image mutex, which is built through init_recursive_pi_mutex precisely because it needs it), and then does a blocking socket write(). So the SCHED_FIFO dispatcher can block behind a low-priority logging thread while holding the image lock, which stalls every plugin thread waiting on that lock. It is also unrate-limited: one line per offending entry, up to DBGW_MAX_ENTRIES = 128 per drain, and an OPC-UA client repeatedly writing one bad address reproduces it every tick.
Suggest keeping the guard but counting instead of logging — a static unsigned g_force_oob_drops reported from off the RT path, or the rate-limited pattern already used elsewhere in plc_state_manager.cpp. Better still, give journal_force_set/journal_force_clear an int return so apply_located can propagate the refusal back to the debugger or OPC-UA client, which is where the person who asked for the force is actually looking.
| .buffer_size = (int)image_tables_capacity(), | ||
| .image_mutex = itm, | ||
| }; | ||
| if (journal_init(&journal_ptrs) != 0) |
There was a problem hiding this comment.
🟡 Changes required · confidence 8/10 · a newly realistic failure is logged and ignored
if (journal_init(&journal_ptrs) != 0)
{
log_error("Failed to initialize journal buffer");
}
else
{
log_info("Journal buffer initialized");
}No return, no ERROR state. That was tolerable while journal_init could only fail on a NULL argument — a programming error caught at first boot. This PR adds force_map_alloc to both journal_init variants, so it now fails on allocation pressure at runtime, and that path leaves g_initialized == false.
With g_initialized == false, every journal_add returns -1 and journal_apply_and_clear returns immediately. That is not only "plugins cannot write": the program's own copy-out goes through the same journal (image_tables.cpp calls journal_write_bool / journal_write_byte there). So the runtime proceeds to plugin_driver_start, takes real-time priority, spawns the task threads and publishes RUNNING with every located output permanently dead, and one log_error line to say so.
That is the same class of silent failure this task set out to remove, one layer up. Suggest treating it as fatal and matching the image-allocation branch this PR itself added further down ("log and stop, never a partial image"): publish ERROR and return from plc_cycle_thread.
| # Left as a parse failure rather than an exception: a | ||
| # garbled line should produce the same clear refusal as an | ||
| # out-of-range one rather than a traceback from the parser. | ||
| sizes[key] = -1 |
There was a problem hiding this comment.
🟡 Changes required · confidence 9/10 · the refusal message describes the wrong mistake
read_image_conf_file stores -1 as the sentinel for an unparseable count:
try:
sizes[key] = int(count)
except ValueError:
sizes[key] = -1validate_table_count then always receives an int, so its except (TypeError, ValueError) branch — the one that says "must be a whole number of {expected}" — is unreachable from the file path, and the -1 falls into the count < 0 branch instead. Running the real module:
int_output=4.5 words -> int_output cannot be negative (got -1).
int_output=<5000 digits> words -> int_output cannot be negative (got -1).
The refusal is correct and nothing escapes, but the message is the entire point of validating here rather than in the core — this module's own docstring says refusing it here is the only place a person sees it. Someone who wrote 4.5 goes looking for a minus sign that is not there, and a completely different mistake (Python's int-from-string digit limit) produces the same sentence.
One-line fix: make the sentinel None instead of -1. int(None) raises TypeError, the existing handler fires, the message becomes "int_output must be a whole number of words.", and the negative branch goes back to meaning an actual negative.
| * stopping the running ones and freeing would leave it pointing at | ||
| * released memory. cleanup_init undoes init() for every plugin, | ||
| * started or not, which is what actually ends the last reference. */ | ||
| plugin_driver_cleanup_init(plugin_driver); |
There was a problem hiding this comment.
❓ Question · confidence 6/10 · raising this for the EtherCAT owner rather than as a blocker
Adding plugin_driver_cleanup_init here is necessary for the free below to be safe, and the reasoning in the comment above it is right. The side effect is that from the first STOP onwards every plugin is de-initialised, where previously they stayed initialised until the next load's plugin_driver_update_config.
plugin_driver_execute_command does not gate on that — it dispatches on plugin->native_plugin && plugin->native_plugin->execute_command with no plugin->initialized check, and native_plugin survives cleanup_init since only teardown_plugin_instance frees it. EtherCAT's cleanup() frees g_masters, nulls it and zeroes g_master_count, but leaves its cached g_runtime_args untouched.
So PLUGIN_CMD handlers issued while the PLC is stopped — which is exactly when the editor scans slaves and reads status — now run against torn-down state. I traced the dispatch but not every handler: scan and list-interfaces look bus-level and independent of g_masters, so I could not demonstrate a crash, but status and diagnostics iterate g_masters and will report nothing where they previously reported the configured masters. The comment a few lines above also notes EtherCAT is "deliberately init'd even when disabled", and this change ends that state at every stop.
Is that intended? If the editor-side commands do need the plugin initialised, either gate plugin_driver_execute_command on plugin->initialized and re-init on demand, or scope the de-init to the plugins that actually cached the image.
… runtime Marcone's review on #195. One blocker and five changes required. USE-AFTER-FREE REACHABLE FROM A PLUGIN THREAD, in two halves. force_map_free freed the fourteen rows and only then cleared g_force_count and g_force_size -- the two variables is_slot_forced checks before indexing -- so a reader that passed both guards dereferenced a NULL row. And journal_cleanup() ran four lines BEFORE plugin_driver_stop, with plugin threads still live: image_lock is handed to every plugin as args->image_lock and calls journal_apply_and_clear, so is_slot_forced runs on EtherCAT's bus thread and the s7comm callback. Forcing a variable from the debugger is all it takes for there to be anything to read. Both halves fixed: the guards go down before the free, and the cleanup moves after the stop. plc_retain_flush() stays before the stop, because a plugin-backed store has to be alive to answer, so the two now sit on opposite sides of it deliberately. THE IMAGE IS NO LONGER GATED ON A PLUGIN CONCERN. Sizing and allocation sat inside `if (plugin_driver)`, and plc_main.c skips its whole plugin block when plugin_driver_create() returns NULL -- no error, no exit. A later START then left capacity at zero for the life of the process: fill_null_pointers looped zero times, journal_init got fourteen null bases, every journal write was rejected and every located read returned zero. The PLC scanned and drove nothing, silently, which is the failure plc_main.c refuses to ship a few lines above its own boot allocation. The block is hoisted out, still after plugin_manager_load and before plugin_driver_init. A FAILED journal_init NOW STOPS THE RUNTIME. It could only fail on a NULL argument before; it allocates the forced-slot map now, so it fails under memory pressure at run time and leaves g_initialized false. Every journal_add then returns -1 -- including the program's own copy-out, which goes through the same journal -- so carrying on published RUNNING with every located output permanently dead and one log line to say so. FORCES OUT OF RANGE ARE COUNTED, NOT LOGGED. Saying it out loud was the right instinct and the wrong place: both force paths run under image_lock on the real-time thread, and log_warn takes a mutex with no priority inheritance before a blocking socket write, so a SCHED_FIFO dispatcher could block behind the logging thread while holding the image lock. Unrate-limited too, at one line per entry. The count is reported once per cycle from outside the lock, and reading it clears it. THE RELEASE HALF OF THE ORDERING IS CHECKED. Allocate-before-init is a real runtime refusal; release-after-stop had only a comment, so a refactor moving a cleanup_init below the free got no diagnostic while both native plugins hold the base pointers they copied by value at init(). A new plugin_driver_any_initialized() guards all three free sites. AND THE REFUSAL MESSAGE NAMES THE RIGHT MISTAKE. An unparseable count was stored as -1, so `int_output=4.5 words` was refused with "cannot be negative (got -1)" and the reader went looking for a minus sign that is not there. The sentinel is None, which reaches the handler that says "must be a whole number of words" -- and refusing it where someone is watching the build log is this module's whole reason for existing. 219 pytest; every changed TU clean under -Wall -Wextra -Werror, both journal variants included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s://github.com/Autonomy-Logic/openplc-runtime into RTOP-284-B-per-table-allocation # Conflicts: # core/src/plc_app/plc_state_manager.cpp
… mappings Marcone's review on #196. One blocker and three changes required. THE FEATURE SHIPPED INERT, which is the blocker and the one that makes everything under it untested rather than merely unverified. The runtime keeps the image square for any run in which even one loaded plugin lacks set_image_sizes -- correctly -- and plugins_default.conf ships five, of which only simple_modbus declared it. So image_sizes_flatten ran on every load of a stock device, and the two enum-order fixes, the three loop fixes and the s7comm/EtherCAT clamps all ran in the degenerate case where they cannot differ from the old behaviour. modbus_master and opcua now declare it. Nothing else was needed: both already bound through SafeBufferAccess -> BufferValidator, which this branch migrated to the table each buffer lives in. A test reads plugins_default.conf and asserts every shipped plugin declares it -- Python by import, native by linking plugin_image_sizes.c. Verified it fails on the state that shipped. Nothing failed before: the image was simply square, which is also what a correct square run looks like, and this is the only thing that tells the two apart. A DEGRADED PLUGIN NO LONGER GETS A VOTE. It failed to load, so plugin_driver_init skips it in every branch: it never receives runtime args and never touches the image. Letting it answer "no" meant one box missing Npcap, where EtherCAT degrades, silently cost every other plugin its per-table image. Disabled plugins still vote, because init runs for them regardless. THE FORCE BOUND AND THE WRITE BOUND AGREE AGAIN. force_map_alloc sizes every row to the LONGEST table so each type has somewhere to record, but journal_force_set validated only against that row length while apply_write_raw and is_slot_forced validate against the table's own. While every table had one length the two were one number; they can now disagree. With bool_output at 1 and int_output at 100, forcing bool_output index 5 passed, flipped the bit and incremented g_force_count -- permanently disabling the fast path -- while both readers refused it. A force that did nothing and said nothing, which is exactly what the guard exists to report. Both force paths now check the table as well as the row. AND THE THREE REMAINING MAPPINGS ARE PINNED. kJournalToImageTable was checked; s7_image_table, ecat_table_for and SEGMENT_TABLES were not, and Ceedling does not run in CI so nothing compile-checks the two C ones either. The contract test now extracts all three by regex and asserts name-to-name correspondence and completeness -- SEGMENT_TABLES for all eight entries, where the behavioural tests happened to exercise three. Verified each catches a deliberately corrupted entry. The fourth copy of the table order is pinned with them: shared/image_sizes.py's IMAGE_TABLE_ORDER is what turns the runtime's positional array into names, and it joins the existing parametrize rather than getting a test of its own. 238 pytest; every changed TU clean under -Wall -Wextra -Werror, both journal variants included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… came from Marcone's nits on #196, plus the question he flagged without calling it a bug. THE JOURNAL BOUNDS BY ITS OWN SNAPSHOT. apply_write_raw read the LIVE image sizes while the table pointers beside it were captured at journal_init, so the bound and the pointers came from two different moments. They do not diverge today -- the image is allocated before the cycle thread that calls journal_init exists, and a re-load stops that thread first -- but the coupling was implicit, and implicit is what this whole task keeps finding. The fourteen lengths are now captured with the pointers, in the journal's own order through an exported journal_type_to_image_table() rather than a second copy of the map. A HALF-DELIVERED SIZE MAP IS NO LONGER LEFT BEHIND. set_image_sizes built into _sizes as it parsed and returned -1 on a bad entry, leaving the tables before the failure answering and the rest falling back. _sizes is process-global -- imported once per interpreter, shared by every Python plugin in the process -- so a plugin being torn down could leave that for plugins still running. It now builds into a local and publishes only on success. AND THREE COMMENTS THAT WERE WRONG: - plugin_driver.c carried the third copy of the buffer_size contract, still saying all fourteen tables are allocated at the same count and pointing at image_sizes_flatten. Every clause was false. It now says what the other two say: the minimum is the only safe single number for a consumer that has not been told the tables can differ. - The deferral of the deprecation attribute was justified with "-Werror". -Werror IS set, in core/src/CMakeLists.txt -- but only for the runtime core. The plugins are configured by their own cmake invocation and the VPP packages by a plain Makefile, so the attribute would warn there rather than fail. The real reason to defer is that the field is still the RIGHT thing to read: on a square run it is the length every table has, and it is the only bound a plugin that has not adopted the symbol can use. Deprecating now would warn at correct code. - "BUFFER_TYPE_INT_MEMORY is 7" was off by one: s7comm_config.h starts at BUFFER_TYPE_NONE = 0, so it is 8. Seven is the journal's. "8 here, 7 in the journal, 10 in the image" is the stronger sentence anyway -- three numbers for one table is the whole argument. Also: PyErr_Clear() on the other three optional Python lookups, so the comment claiming every optional lookup clears it is true rather than aspirational -- a plugin defining set_image_sizes but not cleanup left an AttributeError set on exit. And #undef N moved to just after the last use instead of sitting inside a runtime branch, where it worked only because every use happened to be above it. 239 pytest; every changed TU clean under -Wall -Wextra -Werror, both journal variants and s7comm included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The required half of this finding was done in the previous commit -- the out-of-image force is counted rather than logged on the real-time path. This is the half that was left: the refusal went nowhere, so nothing acted on it. `journal_force_set` and `journal_force_clear` now return int, and `apply_located` uses it. The two pins it sets have to agree or the debugger lies: the image slot can refuse -- address outside the table, or the forced-slot map never allocated -- while `ext_strucpp_debug_set` always accepts, because the IECVar exists whatever the image is sized to. Pinning the IECVar first showed the variable as FORCED in the editor and over OPC UA while the image slot took nothing, so the program kept driving it and the displayed value was fiction. A refusal the user is told is a success is worse than the refusal. So the image goes first and the program view only follows a force that landed. UNFORCE keeps releasing both regardless: a refused clear means the slot cannot have been forced, and holding the IECVar pinned because of it would strand the variable forced with no way to release it. Not propagated further back, and that is a limit rather than an omission. The write was enqueued by `runtime_external_write`, which returned to its caller a cycle before the drain runs, so there is no response channel left from here. Leaving the variable visibly unforced is the only honest signal this path still owns -- and it is the one the person who asked for the force is looking at. Both translation units compile clean under the core's own flags, -Werror included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s://github.com/Autonomy-Logic/openplc-runtime into RTOP-284-B-per-table-allocation
The header already documented `table_sizes[]` as the snapshot taken at journal_init "from the SAME moment as the pointers beside it", and gave the reason: reading the live image sizes while holding pointers captured earlier would, if the two ever diverged, apply a new length to an old allocation. Only `journal_longest_table` actually did that. `journal_type_capacity` -- the one on the drain path, and the one the review asked about -- still called `image_table_capacity()`, which reads `g_sizes`, a global `image_tables_alloc` rewrites under the image-tables mutex this path does not hold. So the intent was written down and the hot-path function never followed it. Every caller uses the result to index one of the snapshot's pointers, so taking the length from a later moment than the allocation it bounds is how a dropped write becomes an out-of-bounds index instead. Not a live bug, and the comment says so: the image is allocated before the cycle thread that calls journal_init exists, and a re-load stops that thread first. It is the implicit coupling made explicit, which is what the rest of this task has been doing -- and it turns a non-inlinable cross-TU call per journal entry on the drain path back into the struct field read it used to be. Also merges #195, whose refused-force change touches the same two functions. Both per-table bounds survive the merge intact. Verified: journal_buffer.c, debug_write_journal.cpp, plc_state_manager.cpp and image_tables.cpp all compile clean under the core's own flags, -Werror included; 59 pytest contract tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cation feat(image): allocate each table at its own length, told to the plugins
2 participants
Summary
BUFFER_SIZEis gone. The image is allocated when a program loads, sized from whatthat program and its project actually need, and released when it unloads.
It was 1024 per table, compiled in, identical for every program that ever ran on the
device. That is wrong in both directions: a project needing more could not have it —
the reporter of openplc-editor#296 has a board with three times the memory and the
same ceiling — and a project needing less paid for the rest anyway, out of the memory
its own program wanted.
Six commits, each one standing on its own:
image_tables_t g_image, andplugin_driver.c's hand-writtenexternblock forthe same fourteen is deleted — redundant while the shapes agreed, two incompatible
declarations in different TUs the moment they stopped, which C does not diagnose
across translation units. The
sizeofon those tables is confined to one functionand pinned by
static_asserts.image.confand its install-time gate.apply_image_confbesideapply_retain_conf, validating at install with a line in the build log, for thereason that function's own docstring already gives.
max(byte_index)+1per table fromthe
.so'slocatedVars[], adopted asmax(configured, derived).plugin_manager_loadandplugin_driver_init.Ticket
RTOP-284 — https://autonomylogic.atlassian.net/browse/RTOP-284
Requirements Gathering, approved v1.2: https://autonomylogic.atlassian.net/wiki/spaces/CD/pages/282886145
Implementation Plan, with the phase breakdown and every decision recorded:
https://autonomylogic.atlassian.net/wiki/spaces/CD/pages/288292865
Editor side, in review: DOPE-615 — Autonomy-Logic/openplc-editor#1093 and
Autonomy-Logic/openplc-web#742. That task emits the sizes this one reads.
Decisions worth a reviewer's attention
One size for all fourteen tables.
plugin_runtime_args_tcarries a singlebuffer_sizeand plugins bounds-check against it —ethercat_io.crefuses abyte_indexat or above it,s7commderives every clamp from it. One number describesfourteen tables only while they are the same size. Give each its own and no value of
that field is right: the minimum makes every plugin refuse everything the moment one
table is empty (a project with
%QW4096and no%IXhas a floor of zero), and themaximum lets a plugin write past the end of the smaller ones — the exact overflow this
work prevents. Per-table sizes need a field per table, which breaks CON06 and
invalidates pre-compiled plugins. So the image is square, at the largest count any
table needs: ~460 KB of pointers for a 4096-word program on 64-bit Linux, against
breaking every shipped plugin.
image.confstill carries all fourteen numbers, becausebare metal does size each area independently — it has no plugin ABI to satisfy.
Placement is the requirement, not the mechanism. The allocation sits after
plugin_manager_load, because the floor comes from walking the loaded.soand thereis no
.sobefore it, and beforeplugin_driver_init, because that is where the basepointers and
buffer_sizeare copied into the runtime args and both native pluginscache that struct by value inside
init(). Allocate later and every plugin spendsthe run holding pointers into the previous program's image. Nothing enforced that
ordering, so building the runtime args now refuses outright when the capacity is zero,
before the pointers are copied — not an
assert(), which vanishes under NDEBUG. Thefailure it prevents is not a crash: plugins would hold null tables and a
buffer_sizeof zero, which every bounds check reads as "refuse every index", presenting as I/O that
silently does nothing.
RSK05, the open question in the requirements, is answered from the code. It asked
whether the Modbus server should fail, warn or clamp when the image is smaller than the
configured exposure. It is a false choice. Each data block declares itself as wide as
its counts, and pymodbus's own
validate()answers exception 02 (Illegal Data Address)past that. A block wider than the image lets the gap pass validate, fail the buffer
read, and answer zero — a plausible wrong value a client cannot tell from a real
one. Clamped to the image the two agree and the protocol reports the truth by itself,
so clamping is reporting once it stops lying about what exists. Failing would lose
the Modbus server entirely over a missing file, and the cause is usually a version
mismatch rather than a user error. A startup warning names what shrank, for the person
who configured it and is not the client.
A case nobody had noticed, and it is the common one.
generateModbusSlaveConfigmaterialises its defaults (1024 registers, 8192 coils) into
modbus_slave.jsonevenwhen the project never opened the Modbus screen — so a project with eight
%QWwoulddeclare 1024 registers over an image of eight. Clamping to the image fixes it with no
editor change.
How it was tested
docker build, which runsinstall.sh --native):compiles and links the whole runtime with the project's own CMake flags.
Built target plc_main, no warnings in any changed file../build/plc_main --print-logsin the container logs[image_tables] image allocated: 1 elements per table— the boot minimum, allocatedbefore
plugin_driver_init— and all five plugins report PASS oninit,start_loop,stop_loopandcleanup. That covers the boot path, the promise thata plugin never receives a null base pointer or a zero size, and the fact that the new
ordering guard does not fire on the correct path.
bash scripts/run-pytest.shterritory: 202 webserver tests and 24 Python-plugintests. 18 new tests for
apply_image_conf(present/absent contract, install-timerefusal, the ABI ceiling, the parser) and 6 for the cross-implementation contract.
-fsyntax-onlyunder-Wall -Wextraon every changed translation unit, includingthe journal in both build variants (lock-free and the mutex fallback — each has
its own
journal_init/journal_cleanup, and fixing one would work only on machineswith lock-free atomics).
static_asserts were verified to fire, with the intended message, byswapping a table to a pointer in a sandbox — and then fired for real when the types
changed in commit 3, naming the function to follow.
A guard for a contract that had none.
image.confis written in three places andread in a fourth: the editor emits it, the webserver validates and installs it, the
core parses it, and
image_tables.hdeclares the tables it names. A key added on oneside and forgotten on another fails nothing — the core never sees that table's size,
falls back to the derived floor, and the image is quietly smaller than the project
asked for.
tests/pytest/plugins/test_image_conf_contract.pychecks the enum, the keyarray and the struct fields against the Python list in order, by reading the C
sources as text. That is deliberate: pytest is the only suite CI runs in this
repository, so it is the only guard that will actually fire. Verified it catches a
reordering.
Checklist
tests/pytest/modbus_master,tests/pytest/plugins/opcuaandtest_openplc_input_registers_datablock.pyare identical on a cleandevelopmentcheckout)pre-commit runclean on the changed filesImplementation Plan
docs/pr-reviews/PR_REVIEW_CHECKLIST.mdTwo things found on the way, neither introduced here
project.yml's-DBUFFER_SIZE=128never took effect whereimage_tables.hwasincluded: the
#definewas unconditional and overrode the command line. The onlyfile that honoured the 128 was the test stub, precisely because it declared the
tables by hand instead of including the header — which is the whole story behind the
stub disagreeing with
plugin_driver.c. Both are fixed here.pre-commit run --all-filesreformats around 200 files in this repository, becauseblackandruff formatdisagree and undo each other. Worth fixing separately; untilthen, run the hooks on your own files.
🤖 Generated with Claude Code