From a9c490607c2e301ff7138ef8b3cf96ac1db038cc Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 09:00:14 -0300 Subject: [PATCH 1/5] feat(image): allocate each table at its own length, told to the plugins 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 --- core/src/drivers/plugin_driver.c | 142 +++++++++- core/src/drivers/plugin_driver.h | 46 +++ core/src/drivers/plugin_types.h | 22 +- .../plugins/native/ethercat/CMakeLists.txt | 4 + .../plugins/native/ethercat/ethercat_io.c | 57 +++- .../plugins/native/ethercat/ethercat_plugin.c | 20 +- .../plugins/native/plugin_image_sizes.c | 37 +++ .../plugins/native/plugin_image_sizes.h | 46 +++ .../plugins/native/s7comm/CMakeLists.txt | 4 + .../plugins/native/s7comm/s7comm_plugin.cpp | 108 ++++++- .../python/examples/example_python_plugin.py | 40 ++- .../python/modbus_slave/simple_modbus.py | 74 ++++- .../plugins/python/shared/buffer_validator.py | 36 ++- .../plugins/python/shared/image_sizes.py | 87 ++++++ core/src/drivers/python_plugin_bridge.h | 4 + core/src/plc_app/image_table_id.h | 53 ++++ core/src/plc_app/image_tables.cpp | 266 +++++++++++++----- core/src/plc_app/image_tables.h | 87 ++---- core/src/plc_app/journal_buffer.c | 98 +++++-- core/src/plc_app/journal_buffer.h | 11 +- core/src/plc_app/plc_main.c | 9 +- core/src/plc_app/plc_state_manager.cpp | 23 +- tests/pytest/test_image_conf_contract.py | 72 ++++- tests/pytest/test_modbus_exposure_fit.py | 57 ++++ 24 files changed, 1184 insertions(+), 219 deletions(-) create mode 100644 core/src/drivers/plugins/native/plugin_image_sizes.c create mode 100644 core/src/drivers/plugins/native/plugin_image_sizes.h create mode 100644 core/src/drivers/plugins/python/shared/image_sizes.py create mode 100644 core/src/plc_app/image_table_id.h diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 1ac0f058..e6073629 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -591,6 +591,107 @@ int plugin_driver_load_config(plugin_driver_t *driver, const char *config_file) } // Send to plugin init function all args +/** + * The fourteen table lengths, for a plugin that asked to be told. + * + * Read once per plugin_driver_init() rather than cached: the image is + * reallocated on every program load, and a cached copy is exactly the stale + * state this symbol exists to prevent. + */ +static uint32_t image_sizes_snapshot(uint32_t *out, uint32_t cap) +{ + uint32_t n = 0; + for (int id = 0; id < IMAGE_TABLE_COUNT && n < cap; ++id) + { + out[n++] = image_table_capacity((image_table_id_t)id); + } + return n; +} + +/** + * Hand one plugin the fourteen lengths, before its init() runs. + * + * Returns 0 when the plugin has nothing to be told or accepted them, and + * non-zero when it refused -- which fails the plugin exactly as a failed + * init() does, because a plugin that cannot make sense of the image it is + * about to be handed should not be handed it. + */ +static int deliver_image_sizes(plugin_instance_t *plugin) +{ + uint32_t sizes[IMAGE_TABLE_COUNT]; + const uint32_t count = image_sizes_snapshot(sizes, IMAGE_TABLE_COUNT); + + if (plugin->config.type == PLUGIN_TYPE_NATIVE && plugin->native_plugin && + plugin->native_plugin->set_image_sizes) + { + const int rc = plugin->native_plugin->set_image_sizes(sizes, count); + if (rc != 0) + { + log_error("Plugin '%s' refused the image sizes (returned %d)", plugin->config.name, rc); + return rc; + } + } + + if (plugin->config.type == PLUGIN_TYPE_PYTHON && plugin->python_plugin && + plugin->python_plugin->pFuncSetImageSizes) + { + PyObject *list = PyList_New((Py_ssize_t)count); + if (!list) + { + PyErr_Clear(); + log_error("Plugin '%s': could not build the image size list", plugin->config.name); + return -1; + } + for (uint32_t i = 0; i < count; ++i) + { + /* PyList_SetItem steals the reference, so a failed PyLong_FromLong + * is the only leak to worry about and it cannot happen for a + * uint32_t that already exists. */ + PyList_SetItem(list, (Py_ssize_t)i, PyLong_FromUnsignedLong(sizes[i])); + } + PyObject *result = + PyObject_CallFunctionObjArgs(plugin->python_plugin->pFuncSetImageSizes, list, NULL); + Py_DECREF(list); + if (!result) + { + PyErr_Print(); + log_error("Plugin '%s' raised in set_image_sizes", plugin->config.name); + return -1; + } + Py_DECREF(result); + } + + return 0; +} + +bool plugin_driver_all_understand_per_table_sizes(plugin_driver_t *driver, + const char **first_without) +{ + if (first_without) + *first_without = NULL; + if (!driver) + return false; + + for (int i = 0; i < driver->plugin_count; i++) + { + plugin_instance_t *plugin = &driver->plugins[i]; + bool understands = false; + + if (plugin->config.type == PLUGIN_TYPE_NATIVE) + understands = plugin->native_plugin && plugin->native_plugin->set_image_sizes; + else if (plugin->config.type == PLUGIN_TYPE_PYTHON) + understands = plugin->python_plugin && plugin->python_plugin->pFuncSetImageSizes; + + if (!understands) + { + if (first_without) + *first_without = plugin->config.name; + return false; + } + } + return true; +} + int plugin_driver_init(plugin_driver_t *driver) { if (!driver) @@ -632,6 +733,18 @@ int plugin_driver_init(plugin_driver_t *driver) } return -1; } + /* BEFORE init(), because init() is where a plugin copies the + * base pointers by value and decides how big everything is. + * Delivering afterwards leaves a window, once per load, in which + * the plugin holds new pointers and previous sizes. */ + if (deliver_image_sizes(plugin) != 0) + { + Py_DECREF(args); + if (have_gil) + PyGILState_Release(local_gstate); + return -1; + } + // Call the Python init function with proper capsule PyObject *result = PyObject_CallFunctionObjArgs(plugin->python_plugin->pFuncInit, args, NULL); @@ -671,6 +784,15 @@ int plugin_driver_init(plugin_driver_t *driver) return -1; } + /* BEFORE init(), for the same reason as the Python path above. */ + if (deliver_image_sizes(plugin) != 0) + { + free_structured_args(args); + if (have_gil) + PyGILState_Release(local_gstate); + return -1; + } + // Call the native init function int result = plugin->native_plugin->init(args); if (result != 0) @@ -1112,7 +1234,7 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * * against this field -- ethercat_io.c refuses a byte_index at or above it, * s7comm derives every clamp from it -- so it has to describe the image * that actually exists. It describes all fourteen tables because they are - * all allocated at the same count; see image_sizes_largest() for why the + * all allocated at the same count; see image_sizes_flatten() for why the * ABI leaves no room for anything else. */ args->buffer_size = (int)image_tables_capacity(); args->bits_per_buffer = 8; @@ -1333,6 +1455,19 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) py_binds->pFuncStop = NULL; } + py_binds->pFuncSetImageSizes = PyObject_GetAttrString(py_binds->pModule, "set_image_sizes"); + if (!py_binds->pFuncSetImageSizes || !PyCallable_Check(py_binds->pFuncSetImageSizes)) + { + /* Optional. PyErr_Clear() is not decoration: a failed + * PyObject_GetAttrString leaves an AttributeError SET, and the next + * CPython call that checks would report this absence as its own + * failure. The four required lookups above never hit it because they + * return on failure. */ + Py_XDECREF(py_binds->pFuncSetImageSizes); + py_binds->pFuncSetImageSizes = NULL; + PyErr_Clear(); + } + py_binds->pFuncCleanup = PyObject_GetAttrString(py_binds->pModule, "cleanup"); if (!py_binds->pFuncCleanup || !PyCallable_Check(py_binds->pFuncCleanup)) { @@ -1475,6 +1610,11 @@ int native_plugin_get_symbols(plugin_instance_t *plugin) native_bundle->retain_load = (plugin_retain_load_func_t)dlsym(handle, "retain_load"); native_bundle->retain_flush = (plugin_retain_flush_func_t)dlsym(handle, "retain_flush"); + /* Optional, like the five above: NULL simply means this plugin does not + * understand per-table image sizes, and the run stays square for it. */ + native_bundle->set_image_sizes = + (plugin_set_image_sizes_func_t)dlsym(handle, "set_image_sizes"); + // Store the native bundle and handle in the plugin instance plugin->native_plugin = native_bundle; diff --git a/core/src/drivers/plugin_driver.h b/core/src/drivers/plugin_driver.h index dac0d063..b8a73bc5 100644 --- a/core/src/drivers/plugin_driver.h +++ b/core/src/drivers/plugin_driver.h @@ -82,6 +82,29 @@ typedef int (*plugin_retain_load_func_t)(const char *program_md5, uint16_t md5_l * assumed to commit inside save(), which is where durability belongs anyway. */ typedef int (*plugin_retain_flush_func_t)(void); +/* Optional, and its PRESENCE is the capability declaration (RTOP-284). + * + * The fourteen image tables no longer share one length, and + * `plugin_runtime_args_t` carries a single `buffer_size` that cannot say so. + * Rather than move that struct -- CON06 guarantees pre-compiled plugins keep + * their field offsets -- the sizes travel through a symbol the loader resolves + * with dlsym, exactly as it already does for execute_command, get_stats and + * the three retain_* hooks. + * + * Exporting it means "I understand per-table sizes". A plugin without it is + * not broken and is not refused; the runtime keeps the image SQUARE for that + * run instead, because a plugin bounding a byte index and a word index with + * the same number is only safe while the tables are equal. + * + * Called BEFORE init(), once per plugin_driver_init(). Self-contained on + * purpose -- no args -- which is what lets it run that early. A non-zero + * return fails the plugin exactly as a failed init() does. + * + * `sizes` is indexed by `image_table_id_t` and `count` is how many entries it + * carries, so a plugin built against an older enum reads the prefix it knows + * and ignores the rest. */ +typedef int (*plugin_set_image_sizes_func_t)(const uint32_t *sizes, uint32_t count); + typedef struct { void *handle; // Handle to the loaded shared library @@ -97,6 +120,9 @@ typedef struct plugin_retain_save_func_t retain_save; plugin_retain_load_func_t retain_load; plugin_retain_flush_func_t retain_flush; + /* Optional; NULL means this plugin does not understand per-table sizes and + * the image stays square for the run. */ + plugin_set_image_sizes_func_t set_image_sizes; } plugin_funct_bundle_t; // Plugin instance structure @@ -187,6 +213,26 @@ int plugin_driver_retain_load(plugin_instance_t *store, const char *program_md5, uint8_t *out, uint16_t cap, uint16_t *out_len); int plugin_driver_retain_flush(plugin_instance_t *store); +/** + * Does every loaded plugin understand per-table image sizes? + * + * Asked once per program load, BEFORE the image is allocated, because the + * answer decides how it is allocated. If any plugin says no, the image stays + * SQUARE for that run -- every table the same length. + * + * That is not a preference. The shipped VPP plugins bound a byte index into + * bool_output and a word index into int_output with the same `buffer_size`: + * the largest table would be an out-of-bounds read on the smaller ones, and + * the smallest would silently drop configured I/O. Only equal lengths keep one + * bound honest. + * + * `first_without` receives the name of the first plugin that does not, so the + * decision can be logged with a reason. It is the only way an operator can + * tell the two modes apart. + */ +bool plugin_driver_all_understand_per_table_sizes(plugin_driver_t *driver, + const char **first_without); + // Route a command to a specific plugin by name (for async commands like scan) int plugin_driver_execute_command(plugin_driver_t *driver, const char *plugin_name, const char *command_json, char *response, size_t response_size); diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 62ca61e6..3b9d7a02 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -229,7 +229,27 @@ typedef struct /* Plugin configuration */ char plugin_specific_config_file_path[256]; - /* Buffer size information */ + /* THE SMALLEST TABLE, NOT THE ONLY ONE (RTOP-284). + * + * The fourteen image tables no longer share a length. This field cannot + * say that -- CON06 guarantees pre-compiled plugins keep their field + * offsets, so it does not move -- and it is now the MINIMUM of the + * fourteen rather than the length they all happened to have. + * + * The minimum is the only safe answer for a consumer that still reads one + * number: bounding by it refuses an index that would have run off the end + * of the shortest table, where bounding by the largest would have read + * past every table below it. Under-permissive, never over. + * + * A plugin that wants the truth exports `set_image_sizes` (plugin_driver.h) + * and receives all fourteen before its init() runs. When every loaded + * plugin does, the image is allocated per table; when any does not, it is + * kept square for that run and this field is again the length they all + * have. + * + * Not marked deprecated yet, deliberately: the build carries -Werror, so + * the attribute would fail the build for every consumer still reading it + * rather than naming them. It goes in once they are migrated. */ int buffer_size; int bits_per_buffer; diff --git a/core/src/drivers/plugins/native/ethercat/CMakeLists.txt b/core/src/drivers/plugins/native/ethercat/CMakeLists.txt index 5126f2b2..54c5dc29 100644 --- a/core/src/drivers/plugins/native/ethercat/CMakeLists.txt +++ b/core/src/drivers/plugins/native/ethercat/CMakeLists.txt @@ -153,6 +153,10 @@ set(PLUGIN_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/ethercat_proc.c ${CMAKE_CURRENT_SOURCE_DIR}/ethercat_iface_state.c ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_logger.c + # Exports set_image_sizes, which is how this plugin declares it + # understands per-table image sizes (RTOP-284). Without it the + # runtime keeps the image square for every run this plugin is in. + ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_image_sizes.c ) # ============================================================================= diff --git a/core/src/drivers/plugins/native/ethercat/ethercat_io.c b/core/src/drivers/plugins/native/ethercat/ethercat_io.c index 97de392a..0531ae42 100644 --- a/core/src/drivers/plugins/native/ethercat/ethercat_io.c +++ b/core/src/drivers/plugins/native/ethercat/ethercat_io.c @@ -19,6 +19,8 @@ */ #include "ethercat_io.h" + +#include "../plugin_image_sizes.h" #include "ethercat_master.h" #include @@ -234,6 +236,31 @@ static int ecat_data_type_expected_iec_size(ecat_data_type_t dt) /** * @brief Return a human-readable name for an iec_size_t value */ +/* (direction, size) -> the image table that stores it. + * + * EtherCAT only ever emits %I and %Q, so there is no memory case to answer. + * The mapping is spelled out rather than arithmetic on the enums: the two + * orders are unrelated and a cast between them would land writes in another + * table's bounds. */ +static image_table_id_t ecat_table_for(iec_dir_t dir, iec_size_t size) +{ + const int in = (dir == IEC_DIR_INPUT); + switch (size) + { + case IEC_SIZE_BIT: + return in ? IMAGE_TABLE_BOOL_INPUT : IMAGE_TABLE_BOOL_OUTPUT; + case IEC_SIZE_BYTE: + return in ? IMAGE_TABLE_BYTE_INPUT : IMAGE_TABLE_BYTE_OUTPUT; + case IEC_SIZE_WORD: + return in ? IMAGE_TABLE_INT_INPUT : IMAGE_TABLE_INT_OUTPUT; + case IEC_SIZE_DWORD: + return in ? IMAGE_TABLE_DINT_INPUT : IMAGE_TABLE_DINT_OUTPUT; + case IEC_SIZE_LWORD: + return in ? IMAGE_TABLE_LINT_INPUT : IMAGE_TABLE_LINT_OUTPUT; + } + return IMAGE_TABLE_COUNT; +} + static const char *iec_size_name(iec_size_t sz) { switch (sz) { @@ -301,13 +328,31 @@ int ecat_io_build_channel_map(const ecat_config_t *config, continue; } - /* Bounds check against PLC buffer size */ - if (iec_loc.byte_index >= args->buffer_size) { + /* Bounds check against THIS LOCATION'S OWN TABLE. + * + * args->buffer_size is now the SMALLEST of the fourteen, so using + * it here would refuse every channel above the shortest table's + * length -- an EtherCAT slave silently losing most of its I/O on a + * project that sizes one area small. The table the location + * actually lands in is the only honest bound (RTOP-284). */ + const image_table_id_t table = ecat_table_for(iec_loc.direction, iec_loc.size); + /* Falls back to args->buffer_size when the sizes were never + * delivered. That is not belt and braces: without it a runtime + * that does not call set_image_sizes -- an older one loading this + * plugin -- leaves every table at zero here and EVERY channel is + * refused, taking the whole bus down silently. On a square run + * buffer_size IS the length every table has, so it is the right + * answer rather than a guess. */ + const uint32_t reach = plugin_image_sizes_known() + ? plugin_image_table_capacity(table) + : (uint32_t)(args->buffer_size > 0 ? args->buffer_size : 0); + if (iec_loc.byte_index < 0 || (uint32_t)iec_loc.byte_index >= reach) + { plugin_logger_warn(logger, - "Slave '%s' channel '%s': IEC location '%s' byte index %d " - "exceeds buffer size %d, skipping", - cfg_slave->name, ch->name, ch->iec_location, - iec_loc.byte_index, args->buffer_size); + "Slave '%s' channel '%s': IEC location '%s' byte index %d " + "is outside the %u element(s) that area has, skipping", + cfg_slave->name, ch->name, ch->iec_location, iec_loc.byte_index, + reach); errors++; continue; } diff --git a/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c b/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c index 7c08c922..782724a4 100644 --- a/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c +++ b/core/src/drivers/plugins/native/ethercat/ethercat_plugin.c @@ -53,11 +53,13 @@ #include "plugin_logger.h" #include "plugin_types.h" #include "ethercat_plugin.h" + +#include "../plugin_image_sizes.h" +#include "cJSON.h" /* JSON parsing for execute_command */ #include "ethercat_config.h" -#include "ethercat_master.h" #include "ethercat_io.h" -#include "soem/soem.h" /* osal_get_monotonic_time, ec_timet */ -#include "cJSON.h" /* JSON parsing for execute_command */ +#include "ethercat_master.h" +#include "soem/soem.h" /* osal_get_monotonic_time, ec_timet */ /* Forward declaration: ecat_bus_thread is defined alongside the bus * loop further down in the file but referenced first by @@ -1198,7 +1200,17 @@ int init(void *args) * land in the runtime journal instead of stderr. */ ecat_config_set_logger(&g_logger); - plugin_logger_info(&g_logger, "Buffer size: %d", g_runtime_args.buffer_size); + /* What this plugin was told, not the deprecated single figure: on a + * per-table run buffer_size is the SMALLEST of the fourteen and says + * nothing about the areas this bus actually reaches. */ + if (!plugin_image_sizes_known()) + plugin_logger_info(&g_logger, "Image sizes: not delivered (square run)"); + else + plugin_logger_info(&g_logger, "Image sizes: bits in %u/%u, words in %u/%u", + plugin_image_table_capacity(IMAGE_TABLE_BOOL_INPUT), + plugin_image_table_capacity(IMAGE_TABLE_BOOL_OUTPUT), + plugin_image_table_capacity(IMAGE_TABLE_INT_INPUT), + plugin_image_table_capacity(IMAGE_TABLE_INT_OUTPUT)); /* Parse ALL master configurations from the JSON file */ const char *config_path = g_runtime_args.plugin_specific_config_file_path; diff --git a/core/src/drivers/plugins/native/plugin_image_sizes.c b/core/src/drivers/plugins/native/plugin_image_sizes.c new file mode 100644 index 00000000..1459dbda --- /dev/null +++ b/core/src/drivers/plugins/native/plugin_image_sizes.c @@ -0,0 +1,37 @@ +#include "plugin_image_sizes.h" + +#include + +/* Zeroed until the runtime calls set_image_sizes(), which it does once per + * plugin_driver_init() and therefore once per program load. Deliberately NOT + * remembered across loads: a cached copy from the previous program is exactly + * the stale state the per-load delivery exists to prevent. */ +static uint32_t g_sizes[IMAGE_TABLE_COUNT]; +static int g_known = 0; + +/** + * Exported for the runtime to find by dlsym. Its presence is the declaration. + * + * `count` is how many entries the runtime sent, which need not be + * IMAGE_TABLE_COUNT: a plugin built against an older enum reads the prefix it + * knows and ignores the rest, and one built against a newer enum leaves the + * tail at zero rather than reading past the array. + */ +int set_image_sizes(const uint32_t *sizes, uint32_t count) +{ + memset(g_sizes, 0, sizeof(g_sizes)); + if (!sizes) return -1; + + const uint32_t n = count < (uint32_t)IMAGE_TABLE_COUNT ? count : (uint32_t)IMAGE_TABLE_COUNT; + for (uint32_t i = 0; i < n; ++i) g_sizes[i] = sizes[i]; + g_known = 1; + return 0; +} + +uint32_t plugin_image_table_capacity(image_table_id_t id) +{ + if (id < 0 || id >= IMAGE_TABLE_COUNT) return 0; + return g_sizes[id]; +} + +int plugin_image_sizes_known(void) { return g_known; } diff --git a/core/src/drivers/plugins/native/plugin_image_sizes.h b/core/src/drivers/plugins/native/plugin_image_sizes.h new file mode 100644 index 00000000..7d4a64bd --- /dev/null +++ b/core/src/drivers/plugins/native/plugin_image_sizes.h @@ -0,0 +1,46 @@ +/** + * The fourteen image table lengths, for a native plugin (RTOP-284). + * + * Linking this file into a plugin gives it two things at once: the exported + * `set_image_sizes` symbol the runtime looks for -- whose PRESENCE is how a + * plugin declares it understands per-table sizes -- and the accessor to read + * back what it was told. + * + * One implementation rather than one per plugin, for the same reason the + * logger is shared: three copies of a fourteen-element cache is three places + * for the indexing to drift, and the whole point of this work is that the + * tables no longer share a length. + * + * A plugin that does NOT link this is not broken. The runtime keeps the image + * square for that run and says which plugin forced it. + */ + +#ifndef PLUGIN_IMAGE_SIZES_H +#define PLUGIN_IMAGE_SIZES_H + +#include "../../../plc_app/image_table_id.h" + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * How long one table is, in its own elements. + * + * Returns 0 before the runtime has delivered the sizes and for an id this + * build does not know, which are the same answer for a caller: an area it + * cannot index into. Bounding against 0 refuses every access, which is the + * safe direction for a plugin asked to act before it has been told anything. + */ +uint32_t plugin_image_table_capacity(image_table_id_t id); + +/** Whether the runtime has delivered the sizes for this load yet. */ +int plugin_image_sizes_known(void); + +#ifdef __cplusplus +} +#endif + +#endif /* PLUGIN_IMAGE_SIZES_H */ diff --git a/core/src/drivers/plugins/native/s7comm/CMakeLists.txt b/core/src/drivers/plugins/native/s7comm/CMakeLists.txt index 706a705d..dc373d10 100644 --- a/core/src/drivers/plugins/native/s7comm/CMakeLists.txt +++ b/core/src/drivers/plugins/native/s7comm/CMakeLists.txt @@ -58,6 +58,10 @@ set(PLUGIN_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/s7comm_plugin.cpp ${CMAKE_CURRENT_SOURCE_DIR}/s7comm_config.c ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_logger.c + # Exports set_image_sizes, which is how this plugin declares it + # understands per-table image sizes (RTOP-284). Without it the + # runtime keeps the image square for every run this plugin is in. + ${OPENPLC_ROOT}/core/src/drivers/plugins/native/plugin_image_sizes.c ) # ============================================================================= diff --git a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp index cd11024e..7525a815 100644 --- a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp +++ b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp @@ -32,6 +32,8 @@ extern "C" { #include "plugin_logger.h" #include "plugin_types.h" #include "s7comm_plugin.h" + +#include "../plugin_image_sizes.h" #include "s7comm_config.h" } @@ -335,7 +337,32 @@ extern "C" int init(void *args) plugin_logger_init(&g_logger, "S7COMM", args); plugin_logger_info(&g_logger, "Initializing S7Comm plugin..."); - plugin_logger_info(&g_logger, "Buffer size: %d", g_runtime_args.buffer_size); + /* What this plugin was actually told, not the deprecated single figure. + * On a per-table run `buffer_size` is the SMALLEST of the fourteen and + * says nothing useful about the areas this server serves. Only the + * non-empty tables, because fourteen figures of which most are one reads + * as noise. Nothing parses this line. */ + { + char sizes[192]; + int at = 0; + for (int id = 0; id < IMAGE_TABLE_COUNT && at < (int)sizeof(sizes) - 1; ++id) + { + const uint32_t n = plugin_image_table_capacity((image_table_id_t)id); + if (n <= 1) + continue; + const int wrote = + snprintf(sizes + at, sizeof(sizes) - (size_t)at, "%s%d:%u", at ? " " : "", id, n); + if (wrote < 0 || wrote >= (int)sizeof(sizes) - at) + break; + at += wrote; + } + if (!plugin_image_sizes_known()) + plugin_logger_info(&g_logger, "Image sizes: not delivered (square run)"); + else if (at == 0) + plugin_logger_info(&g_logger, "Image sizes: every table at the minimum"); + else + plugin_logger_info(&g_logger, "Image sizes by table id: %s", sizes); + } g_initialized = true; return 0; @@ -725,6 +752,69 @@ static int get_type_size(s7comm_buffer_type_t type) * ============================================================================= */ +/* s7comm_buffer_type_t -> the image table that stores it. + * + * A THIRD order for the same fourteen tables. This enum groups each width's + * memory beside its input and output, matching journal_buffer_type_t; + * image_table_id_t puts every memory table at the end. BUFFER_TYPE_INT_MEMORY + * is 7 and IMAGE_TABLE_INT_MEMORY is 10, so a cast between them reads and + * writes under another table's bounds. Written out rather than computed. */ +static image_table_id_t s7_image_table(s7comm_buffer_type_t type) +{ + switch (type) + { + case BUFFER_TYPE_BOOL_INPUT: + return IMAGE_TABLE_BOOL_INPUT; + case BUFFER_TYPE_BOOL_OUTPUT: + return IMAGE_TABLE_BOOL_OUTPUT; + case BUFFER_TYPE_BOOL_MEMORY: + return IMAGE_TABLE_BOOL_MEMORY; + case BUFFER_TYPE_BYTE_INPUT: + return IMAGE_TABLE_BYTE_INPUT; + case BUFFER_TYPE_BYTE_OUTPUT: + return IMAGE_TABLE_BYTE_OUTPUT; + case BUFFER_TYPE_INT_INPUT: + return IMAGE_TABLE_INT_INPUT; + case BUFFER_TYPE_INT_OUTPUT: + return IMAGE_TABLE_INT_OUTPUT; + case BUFFER_TYPE_INT_MEMORY: + return IMAGE_TABLE_INT_MEMORY; + case BUFFER_TYPE_DINT_INPUT: + return IMAGE_TABLE_DINT_INPUT; + case BUFFER_TYPE_DINT_OUTPUT: + return IMAGE_TABLE_DINT_OUTPUT; + case BUFFER_TYPE_DINT_MEMORY: + return IMAGE_TABLE_DINT_MEMORY; + case BUFFER_TYPE_LINT_INPUT: + return IMAGE_TABLE_LINT_INPUT; + case BUFFER_TYPE_LINT_OUTPUT: + return IMAGE_TABLE_LINT_OUTPUT; + case BUFFER_TYPE_LINT_MEMORY: + return IMAGE_TABLE_LINT_MEMORY; + /* An unmapped block. IMAGE_TABLE_COUNT is not a table, so + * plugin_image_table_capacity() answers 0 and every access is refused -- + * which is what an unconfigured block should do. */ + case BUFFER_TYPE_NONE: + break; + } + return IMAGE_TABLE_COUNT; +} + +/* How far the table behind `type` reaches, as a signed count so the clamps + * below can subtract a start offset without wrapping. */ +static int s7_table_reach(s7comm_buffer_type_t type) +{ + /* Falls back to the single figure when the sizes were never delivered. + * Without it, a runtime that does not call set_image_sizes -- an older one + * loading this plugin -- leaves every table at zero and every read and + * write is clamped to nothing, so the server answers zeros for everything + * with no error anywhere. On a square run buffer_size IS the length every + * table has. */ + if (!plugin_image_sizes_known()) + return g_runtime_args.buffer_size; + return (int)plugin_image_table_capacity(s7_image_table(type)); +} + /** * @brief Read OpenPLC bool buffer to destination (mutex must be held) */ @@ -750,7 +840,7 @@ static void read_openplc_bool_to_buffer(uint8_t *dest, int size, s7comm_buffer_t return; } - int max_bytes = g_runtime_args.buffer_size - start_buffer; + int max_bytes = s7_table_reach(type) - start_buffer; if (max_bytes > size) max_bytes = size; for (int byte_idx = 0; byte_idx < max_bytes; byte_idx++) { @@ -789,7 +879,7 @@ static void read_openplc_int_to_buffer(uint8_t *dest, int size, s7comm_buffer_ty uint16_t *s7_words = (uint16_t *)dest; int num_words = size / 2; - int max_words = g_runtime_args.buffer_size - start_buffer; + int max_words = s7_table_reach(type) - start_buffer; if (max_words > num_words) max_words = num_words; for (int i = 0; i < max_words; i++) { @@ -823,7 +913,7 @@ static void read_openplc_dint_to_buffer(uint8_t *dest, int size, s7comm_buffer_t uint32_t *s7_dwords = (uint32_t *)dest; int num_dwords = size / 4; - int max_dwords = g_runtime_args.buffer_size - start_buffer; + int max_dwords = s7_table_reach(type) - start_buffer; if (max_dwords > num_dwords) max_dwords = num_dwords; for (int i = 0; i < max_dwords; i++) { @@ -857,7 +947,7 @@ static void read_openplc_lint_to_buffer(uint8_t *dest, int size, s7comm_buffer_t uint64_t *s7_lwords = (uint64_t *)dest; int num_lwords = size / 8; - int max_lwords = g_runtime_args.buffer_size - start_buffer; + int max_lwords = s7_table_reach(type) - start_buffer; if (max_lwords > num_lwords) max_lwords = num_lwords; for (int i = 0; i < max_lwords; i++) { @@ -919,7 +1009,7 @@ static void write_bool_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ int journal_type = map_to_journal_type(type); if (journal_type < 0) return; - int max_bytes = g_runtime_args.buffer_size - start_buffer; + int max_bytes = s7_table_reach(type) - start_buffer; if (max_bytes > size) max_bytes = size; for (int byte_idx = 0; byte_idx < max_bytes; byte_idx++) { @@ -942,7 +1032,7 @@ static void write_int_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_t uint16_t *s7_words = (uint16_t *)src; int num_words = size / 2; - int max_words = g_runtime_args.buffer_size - start_buffer; + int max_words = s7_table_reach(type) - start_buffer; if (max_words > num_words) max_words = num_words; for (int i = 0; i < max_words; i++) { @@ -961,7 +1051,7 @@ static void write_dint_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ uint32_t *s7_dwords = (uint32_t *)src; int num_dwords = size / 4; - int max_dwords = g_runtime_args.buffer_size - start_buffer; + int max_dwords = s7_table_reach(type) - start_buffer; if (max_dwords > num_dwords) max_dwords = num_dwords; for (int i = 0; i < max_dwords; i++) { @@ -980,7 +1070,7 @@ static void write_lint_to_openplc_journal(uint8_t *src, int size, s7comm_buffer_ uint64_t *s7_lwords = (uint64_t *)src; int num_lwords = size / 8; - int max_lwords = g_runtime_args.buffer_size - start_buffer; + int max_lwords = s7_table_reach(type) - start_buffer; if (max_lwords > num_lwords) max_lwords = num_lwords; for (int i = 0; i < max_lwords; i++) { diff --git a/core/src/drivers/plugins/python/examples/example_python_plugin.py b/core/src/drivers/plugins/python/examples/example_python_plugin.py index 5ed49e7c..cbb1133b 100644 --- a/core/src/drivers/plugins/python/examples/example_python_plugin.py +++ b/core/src/drivers/plugins/python/examples/example_python_plugin.py @@ -11,8 +11,9 @@ import threading import sys import os + # Add the parent directory to Python path to find shared module -sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # Import the correct type definitions from shared import ( @@ -20,7 +21,7 @@ safe_extract_runtime_args_from_capsule, SafeBufferAccess, SafeLoggingAccess, - PluginStructureValidator + PluginStructureValidator, ) # Global variable to track initialization @@ -31,6 +32,7 @@ _mainthread = None _stop = threading.Event() + def init(runtime_args_capsule): """ Plugin initialization function @@ -63,8 +65,11 @@ def init(runtime_args_capsule): if _safe_logging_access.is_valid: success, msg = _safe_logging_access.log_info("Python plugin initialization started") if success: - _safe_logging_access.log_debug("Plugin received buffer_size={}, bits_per_buffer={}".format( - runtime_args.buffer_size, runtime_args.bits_per_buffer)) + _safe_logging_access.log_debug( + "Plugin received buffer_size={}, bits_per_buffer={}".format( + runtime_args.buffer_size, runtime_args.bits_per_buffer + ) + ) else: print(f"(WARN) Logging failed: {msg}") else: @@ -75,19 +80,23 @@ def init(runtime_args_capsule): if buffer_size == -1: print(f"(FAIL) Failed to access buffer size: {size_error}") if _safe_logging_access.is_valid: - _safe_logging_access.log_error("Failed to access buffer size: %s", size_error) + _safe_logging_access.log_error(f"Failed to access buffer size: {size_error}") return False - print(f" Buffer size: {buffer_size}") - print(f" Bits per buffer: {runtime_args.bits_per_buffer}") - print(f" Structure details: {runtime_args}") + # Through the logger, not print(). This file is what vendors copy, and + # a print() from a plugin goes to whatever stdout the runtime happens + # to have rather than the central log the operator is reading. + _safe_logging_access.log_info(f"Buffer size (smallest table): {buffer_size}") + _safe_logging_access.log_info(f"Bits per buffer: {runtime_args.bits_per_buffer}") # Create safe buffer access wrapper _safe_buffer_access = SafeBufferAccess(runtime_args) if not _safe_buffer_access.is_valid: print(f"(FAIL) Failed to create safe buffer access: {_safe_buffer_access.error_msg}") if _safe_logging_access.is_valid: - _safe_logging_access.log_error("Failed to create safe buffer access: %s", _safe_buffer_access.error_msg) + _safe_logging_access.log_error( + f"Failed to create safe buffer access: {_safe_buffer_access.error_msg}" + ) return False # Store runtime args for later use @@ -96,7 +105,9 @@ def init(runtime_args_capsule): print("(PASS) Plugin initialized successfully") if _safe_logging_access.is_valid: - success, msg = _safe_logging_access.log_info("Python plugin initialization completed successfully") + success, msg = _safe_logging_access.log_info( + "Python plugin initialization completed successfully" + ) if not success: print(f"(WARN) Final logging failed: {msg}") @@ -105,27 +116,30 @@ def init(runtime_args_capsule): except Exception as e: print(f"(FAIL) Plugin initialization failed: {e}") import traceback + traceback.print_exc() return False + def start_loop(): """ Called when the plugin loop should start Optional function - not all plugins need this """ + def loop(): global _runtime_args, _stop print("Plugin start_loop called") while not _stop.is_set(): time.sleep(1) continue - global _mainthread _mainthread = threading.Thread(target=loop, daemon=True) _mainthread.start() return 0 + def stop_loop(): """ Called when the plugin loop should stop @@ -141,6 +155,7 @@ def stop_loop(): _mainthread = None print("(PASS) Main thread stopped") + def cleanup(): """ Plugin cleanup function @@ -158,11 +173,12 @@ def cleanup(): print("(PASS) Plugin cleaned up successfully") + if __name__ == "__main__": print("This is an example Python plugin for OpenPLC Runtime") print("Expected functions:") print(" - init(runtime_args_capsule) -> bool") - print(" - start_loop() -> None (optional)") + print(" - start_loop() -> None (optional)") print(" - stop_loop() -> None (optional)") print(" - run_cycle() -> None (optional)") print(" - cleanup() -> None (optional)") diff --git a/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py b/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py index 8793fee0..7a6a02f4 100644 --- a/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py +++ b/core/src/drivers/plugins/python/modbus_slave/simple_modbus.py @@ -82,6 +82,13 @@ safe_extract_runtime_args_from_capsule, ) +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). Without it the +# runtime keeps the image square for every run this plugin is loaded in -- +# which, since it is loaded on most devices, would be every run. +from shared.image_sizes import set_image_sizes, table_capacity # noqa: E402,F401 + class OpenPLCDeviceContext(ModbusDeviceContext): """ @@ -974,21 +981,62 @@ def _trim_to_one_table(counts, layout): total -= drop * width +# THE COMPOSED BLOCK, DECIDED (RTOP-284, C1.4) +# +# The holding-register block lays four segments end to end -- qw | mw | 2*md | +# 4*ml -- so it spans four tables in one contiguous Modbus range. The question +# was what "clamp against its own table" means once those four differ in +# length, with three answers on the table: the lowest common extent, truncate +# at the first segment that runs out, or a non-contiguous layout. +# +# It is none of them, because the premise is wrong. The block already dispatches +# each ADDRESS to exactly one segment (OpenPLCSegmentedHoldingRegistersDataBlock +# keeps qw_end, mw_start/end, md_start/end, ml_start/end), and therefore to +# exactly one table. So each segment is clamped against its own table and the +# block is the concatenation of what is left. The other three answers all throw +# away storage the project asked for: the lowest common extent shrinks segments +# that have room, and truncating at the first exhausted one drops segments that +# have their own. +# +# THE CONSEQUENCE FOR THE CLIENT, which is the part worth stating: a segment's +# START ADDRESS moves when a table before it shrinks. That is not new -- the +# layout has always been a function of the counts, so it already moved whenever +# the user changed one -- and the address map screen in the editor is what +# tells them where each segment begins. What IS new is that the counts can now +# change because the project changed, without anyone editing the Modbus screen. +# +# Which image table each Modbus segment actually lives in. The clamp follows +# this rather than one figure, because the tables no longer share a length +# (RTOP-284): %QW comes out of int_output and %ML out of lint_memory, and a +# project may size those very differently. +SEGMENT_TABLES = { + "qw_count": "int_output", + "mw_count": "int_memory", + "md_count": "dint_memory", + "ml_count": "lint_memory", + "qx_bits": "bool_output", + "mx_bits": "bool_memory", + "ix_bits": "bool_input", + "iw_count": "int_input", +} + + +def _segment_limit(key, buffer_size): + """How much of its own table this segment may expose. + + Falls back to `buffer_size` when the runtime did not deliver per-table + sizes, because on a square run that IS the length every table has. + + Bit segments are in bits and their table is in elements of eight, which is + the same conversion the image.conf format makes explicit. + """ + reach = table_capacity(SEGMENT_TABLES[key], buffer_size) + return reach * MAX_BITS if key.endswith("_bits") else reach + + def _fit_counts(asked, buffer_size): """Fit the requested exposure to the image, then to the address space.""" - reg_limit = buffer_size - bit_limit = buffer_size * MAX_BITS - - fitted = { - "qw_count": min(asked["qw_count"], reg_limit), - "mw_count": min(asked["mw_count"], reg_limit), - "md_count": min(asked["md_count"], reg_limit), - "ml_count": min(asked["ml_count"], reg_limit), - "qx_bits": min(asked["qx_bits"], bit_limit), - "mx_bits": min(asked["mx_bits"], bit_limit), - "ix_bits": min(asked["ix_bits"], bit_limit), - "iw_count": min(asked["iw_count"], reg_limit), - } + fitted = {key: min(asked[key], _segment_limit(key, buffer_size)) for key in SEGMENT_TABLES} # THE IMAGE CEILING IS NOT THE PROTOCOL'S CEILING. Fitting each segment to # the image allows 65536 apiece, and the register block composes four of diff --git a/core/src/drivers/plugins/python/shared/buffer_validator.py b/core/src/drivers/plugins/python/shared/buffer_validator.py index 103107b9..92419b39 100644 --- a/core/src/drivers/plugins/python/shared/buffer_validator.py +++ b/core/src/drivers/plugins/python/shared/buffer_validator.py @@ -9,12 +9,14 @@ try: # Try relative imports first (when used as package) - from .component_interfaces import IBufferValidator from .buffer_types import get_buffer_types + from .component_interfaces import IBufferValidator + from .image_sizes import table_capacity except ImportError: # Fall back to absolute imports (when testing standalone) - from component_interfaces import IBufferValidator from buffer_types import get_buffer_types + from component_interfaces import IBufferValidator + from image_sizes import table_capacity class BufferValidator(IBufferValidator): @@ -56,8 +58,17 @@ def validate_buffer_index(self, buffer_idx: int, buffer_type: str) -> Tuple[bool if buffer_idx < 0: return False, f"Buffer index cannot be negative: {buffer_idx}" - if buffer_idx >= self.args.buffer_size: - return False, f"Buffer index out of range: {buffer_idx} >= {self.args.buffer_size}" + # Against THIS BUFFER'S OWN TABLE. args.buffer_size is now the + # smallest of the fourteen (RTOP-284), so validating against it + # would refuse every index above the shortest table's length in + # every longer one. When the sizes were not delivered -- a square + # run -- buffer_size IS the length they all have, so it is the + # right fallback rather than a guess. + reach = table_capacity(buffer_type, self.args.buffer_size) + if buffer_idx >= reach: + return False, ( + f"Buffer index out of range for {buffer_type}: {buffer_idx} >= {reach}" + ) return True, "Success" @@ -103,7 +114,7 @@ def validate_value_range(self, value: Any, buffer_type: str) -> Tuple[bool, str] min_val, max_val = buffer_type_obj.value_range # Handle boolean values - if buffer_type_obj.name == 'bool': + if buffer_type_obj.name == "bool": if isinstance(value, bool): return True, "Success" elif isinstance(value, (int, float)): @@ -132,8 +143,9 @@ def validate_value_range(self, value: Any, buffer_type: str) -> Tuple[bool, str] except (AttributeError, TypeError, ValueError) as e: return False, f"Value validation error: {e}" - def validate_operation_params(self, buffer_type: str, buffer_idx: int, - bit_idx: Optional[int] = None, value: Any = None) -> Tuple[bool, str]: + def validate_operation_params( + self, buffer_type: str, buffer_idx: int, bit_idx: Optional[int] = None, value: Any = None + ) -> Tuple[bool, str]: """ Comprehensive validation of all operation parameters. @@ -214,10 +226,10 @@ def get_validation_summary(self) -> dict: """ try: return { - 'buffer_size': self.args.buffer_size, - 'bits_per_buffer': self.args.bits_per_buffer, - 'supported_buffer_types': list(self.buffer_types.get_all_buffers().keys()), - 'supported_base_types': list(self.buffer_types.get_all_types().keys()) + "buffer_size": self.args.buffer_size, + "bits_per_buffer": self.args.bits_per_buffer, + "supported_buffer_types": list(self.buffer_types.get_all_buffers().keys()), + "supported_base_types": list(self.buffer_types.get_all_types().keys()), } except (AttributeError, TypeError) as e: - return {'error': str(e)} + return {"error": str(e)} diff --git a/core/src/drivers/plugins/python/shared/image_sizes.py b/core/src/drivers/plugins/python/shared/image_sizes.py new file mode 100644 index 00000000..56e8526a --- /dev/null +++ b/core/src/drivers/plugins/python/shared/image_sizes.py @@ -0,0 +1,87 @@ +"""The fourteen image table lengths, for a Python plugin (RTOP-284). + +Importing ``set_image_sizes`` from here into a plugin module does two things at +once: it puts the name in that module's namespace, which is where the runtime +looks for it (``PyObject_GetAttrString(pModule, "set_image_sizes")``), and its +PRESENCE is how the plugin declares it understands per-table sizes. + + from shared.image_sizes import set_image_sizes, table_capacity + +A plugin that does not import it is not broken. The runtime keeps the image +square for that run and logs which plugin forced it -- which is the honest +outcome, because a plugin bounding a byte index and a word index with one +``buffer_size`` is correct exactly while the tables are equal. + +One implementation rather than one per plugin, for the same reason the native +side links one file: three copies of a fourteen-element cache is three places +for the indexing to drift. +""" + +# The order the runtime sends them in, which is the declaration order of +# image_tables_t. Names rather than indices on this side, because the Python +# buffer names already are these names -- see shared/buffer_types.py -- so +# nothing has to know the numbering. +# +# NOTE the trap this avoids: journal_buffer.h and the s7comm plugin each have +# their own fourteen in a DIFFERENT order. Going by name cannot pick up the +# wrong table; going by index can, and silently. +IMAGE_TABLE_ORDER: list[str] = [ + "bool_input", + "bool_output", + "byte_input", + "byte_output", + "int_input", + "int_output", + "dint_input", + "dint_output", + "lint_input", + "lint_output", + "int_memory", + "dint_memory", + "lint_memory", + "bool_memory", +] + +_sizes: dict[str, int] = {} + + +def set_image_sizes(sizes) -> int: + """Receive the table lengths, before ``init`` runs, once per program load. + + Deliberately not remembered across loads: the runtime calls this on every + load precisely so a plugin never acts on the previous program's shape. + + A list shorter than the fourteen leaves the rest unknown rather than + guessed, and a longer one is truncated, so a plugin and a runtime built + against different table lists still agree about the tables they share. + + Returns 0 on success, which is what the runtime requires; non-zero fails + the plugin exactly as a failed ``init`` does. + """ + _sizes.clear() + try: + values = list(sizes) + except TypeError: + return -1 + + for name, count in zip(IMAGE_TABLE_ORDER, values): + try: + _sizes[name] = int(count) + except (TypeError, ValueError): + return -1 + return 0 + + +def sizes_known() -> bool: + """Whether the runtime has delivered the sizes for this load.""" + return bool(_sizes) + + +def table_capacity(name: str, default: int | None = None) -> int | None: + """How long one table is, by the name the buffer accessors already use. + + ``default`` is what to answer when the sizes have not been delivered -- + normally the caller's own ``buffer_size``, which on a square run is the + length every table has anyway. + """ + return _sizes.get(name, default) diff --git a/core/src/drivers/python_plugin_bridge.h b/core/src/drivers/python_plugin_bridge.h index 7ccba5e4..b5caa601 100644 --- a/core/src/drivers/python_plugin_bridge.h +++ b/core/src/drivers/python_plugin_bridge.h @@ -27,6 +27,10 @@ typedef struct PyObject *pFuncStart; PyObject *pFuncStop; PyObject *pFuncCleanup; + /* Optional: set_image_sizes(sizes). NULL when the module does not define + * it, which declares that it does not understand per-table image sizes + * (RTOP-284). See plugin_driver.h for why presence is the declaration. */ + PyObject *pFuncSetImageSizes; PyObject *args_capsule; // Capsule containing plugin_runtime_args_t for lifetime management } python_binds_t; diff --git a/core/src/plc_app/image_table_id.h b/core/src/plc_app/image_table_id.h new file mode 100644 index 00000000..10940cb0 --- /dev/null +++ b/core/src/plc_app/image_table_id.h @@ -0,0 +1,53 @@ +/** + * The identity of each I/O image table, and nothing else. + * + * Split out of image_tables.h so it can reach BOTH sides: the runtime, which + * owns the tables, and plugin_types.h, which plugins include. A plugin that + * exports `set_image_sizes` receives an array indexed by this enum, so it has + * to be able to name the entries -- and the alternative, a second copy of the + * enum in the plugin-facing header, is the drift this file exists to prevent. + * + * Publishing a TYPE costs no ABI: no struct gains a field and no offset moves, + * which is what CON06 guarantees pre-compiled plugins. + * + * ORDER IS THE CONTRACT. It is the declaration order of `image_tables_t`, the + * order `image.conf` is written in, and the order the sizes array arrives in. + * A pytest checks it against the editor's list and the webserver's + * (tests/pytest/test_image_conf_contract.py). Note that journal_buffer.h has + * its own fourteen in a DIFFERENT order -- see kJournalToImageTable. + */ + +#ifndef IMAGE_TABLE_ID_H +#define IMAGE_TABLE_ID_H + +#ifdef __cplusplus +extern "C" { +#endif + +/* One id per table, in the order image_tables_t declares them. Note the gap + * the list makes visible: byte_input and byte_output exist, byte_memory does + * not, so `%MB` has no storage on this runtime at all. */ +typedef enum +{ + IMAGE_TABLE_BOOL_INPUT = 0, + IMAGE_TABLE_BOOL_OUTPUT, + IMAGE_TABLE_BYTE_INPUT, + IMAGE_TABLE_BYTE_OUTPUT, + IMAGE_TABLE_INT_INPUT, + IMAGE_TABLE_INT_OUTPUT, + IMAGE_TABLE_DINT_INPUT, + IMAGE_TABLE_DINT_OUTPUT, + IMAGE_TABLE_LINT_INPUT, + IMAGE_TABLE_LINT_OUTPUT, + IMAGE_TABLE_INT_MEMORY, + IMAGE_TABLE_DINT_MEMORY, + IMAGE_TABLE_LINT_MEMORY, + IMAGE_TABLE_BOOL_MEMORY, + IMAGE_TABLE_COUNT +} image_table_id_t; + +#ifdef __cplusplus +} +#endif + +#endif /* IMAGE_TABLE_ID_H */ diff --git a/core/src/plc_app/image_tables.cpp b/core/src/plc_app/image_tables.cpp index 0977ddbe..a934f0f8 100644 --- a/core/src/plc_app/image_tables.cpp +++ b/core/src/plc_app/image_tables.cpp @@ -44,6 +44,14 @@ image_tables_t g_image; // allocator that sets it. static uint32_t g_capacity = 0; +/* The fourteen counts the image was actually allocated at. + * + * g_capacity survives as the SMALLEST of them, which is what a consumer that + * still reads one number must be given: bounding by the smallest table refuses + * an index that would have run off the end of it, where bounding by the + * largest would have read past four of them. See image_tables_capacity(). */ +static image_sizes_t g_sizes; + // The tables are heap pointers now, and these assertions are what got us here // safely. In their previous form they pinned the inline-array shape, so the // moment the types changed the build stopped and named the function to follow. @@ -845,7 +853,12 @@ uint64_t threaded_image_read(const strucpp::LocatedVar &v) { uint16_t bi = v.byte_index; uint8_t b = v.bit_index; - if (bi >= g_capacity) return 0; + /* The bound is THIS VAR'S TABLE, not one figure for fourteen. With the + * tables at different lengths, g_capacity (the smallest) would refuse + * valid indices in every longer table, and the largest would have let an + * index run off the end of every shorter one. */ + if (bi >= image_table_capacity(table_for(v.area, v.size))) + return 0; switch (v.area) { case strucpp::LocatedArea::Input: @@ -1016,15 +1029,24 @@ static const uint32_t IMAGE_MIN_ELEMENTS = 1; extern "C" uint32_t image_tables_capacity(void) { return g_capacity; } -extern "C" uint32_t image_sizes_largest(const image_sizes_t *sizes) +extern "C" uint32_t image_table_capacity(image_table_id_t id) { - if (!sizes) return 0; + if (id < 0 || id >= IMAGE_TABLE_COUNT) + return 0; + return g_sizes.elements[id]; +} + +extern "C" void image_sizes_flatten(image_sizes_t *sizes) +{ + if (!sizes) + return; uint32_t largest = 0; for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) { if (sizes->elements[i] > largest) largest = sizes->elements[i]; } - return largest; + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + sizes->elements[i] = largest; } extern "C" void image_tables_free(void) @@ -1079,11 +1101,31 @@ extern "C" void image_tables_free(void) temp_lint_memory = nullptr; g_capacity = 0; + std::memset(&g_sizes, 0, sizeof(g_sizes)); } -extern "C" bool image_tables_alloc(uint32_t elements) +extern "C" bool image_tables_alloc(const image_sizes_t *sizes) { - if (elements < IMAGE_MIN_ELEMENTS) elements = IMAGE_MIN_ELEMENTS; + /* A FLOOR OF ONE PER TABLE, not zero, and it is worth being explicit about + * why: an area the program never touches could allocate nothing at all and + * save eight bytes, but then its base pointer is NULL and every plugin + * that does not check the count first dereferences it. FR15 says no plugin + * ever receives an invalid image, including before a program is loaded. + * One element per table costs a pointer and removes that whole class of + * bug; "an area with no producers consumes nothing" (NFR04) is still true + * of the storage that matters, which is the temp buffers and the slots. */ + image_sizes_t want; + if (sizes) + want = *sizes; + else + std::memset(&want, 0, sizeof(want)); + for (int i = 0; i < IMAGE_TABLE_COUNT; ++i) + { + if (want.elements[i] < IMAGE_MIN_ELEMENTS) + want.elements[i] = IMAGE_MIN_ELEMENTS; + } + +#define N(id) (want.elements[id]) /* BUILT INTO LOCALS AND PUBLISHED ONLY ON SUCCESS. * @@ -1116,35 +1158,37 @@ extern "C" bool image_tables_alloc(uint32_t elements) IEC_UDINT *t_dint_memory = nullptr; IEC_ULINT *t_lint_memory = nullptr; - next.bool_input = (IEC_BOOL * (*)[8]) calloc(elements, sizeof(IEC_BOOL *[8])); - next.bool_output = (IEC_BOOL * (*)[8]) calloc(elements, sizeof(IEC_BOOL *[8])); - next.bool_memory = (IEC_BOOL * (*)[8]) calloc(elements, sizeof(IEC_BOOL *[8])); - next.byte_input = (IEC_BYTE **)calloc(elements, sizeof(IEC_BYTE *)); - next.byte_output = (IEC_BYTE **)calloc(elements, sizeof(IEC_BYTE *)); - next.int_input = (IEC_UINT **)calloc(elements, sizeof(IEC_UINT *)); - next.int_output = (IEC_UINT **)calloc(elements, sizeof(IEC_UINT *)); - next.dint_input = (IEC_UDINT **)calloc(elements, sizeof(IEC_UDINT *)); - next.dint_output = (IEC_UDINT **)calloc(elements, sizeof(IEC_UDINT *)); - next.lint_input = (IEC_ULINT **)calloc(elements, sizeof(IEC_ULINT *)); - next.lint_output = (IEC_ULINT **)calloc(elements, sizeof(IEC_ULINT *)); - next.int_memory = (IEC_UINT **)calloc(elements, sizeof(IEC_UINT *)); - next.dint_memory = (IEC_UDINT **)calloc(elements, sizeof(IEC_UDINT *)); - next.lint_memory = (IEC_ULINT **)calloc(elements, sizeof(IEC_ULINT *)); - - t_bool_input = (IEC_BOOL(*)[8])calloc(elements, sizeof(IEC_BOOL[8])); - t_bool_output = (IEC_BOOL(*)[8])calloc(elements, sizeof(IEC_BOOL[8])); - t_bool_memory = (IEC_BOOL(*)[8])calloc(elements, sizeof(IEC_BOOL[8])); - t_byte_input = (IEC_BYTE *)calloc(elements, sizeof(IEC_BYTE)); - t_byte_output = (IEC_BYTE *)calloc(elements, sizeof(IEC_BYTE)); - t_int_input = (IEC_UINT *)calloc(elements, sizeof(IEC_UINT)); - t_int_output = (IEC_UINT *)calloc(elements, sizeof(IEC_UINT)); - t_dint_input = (IEC_UDINT *)calloc(elements, sizeof(IEC_UDINT)); - t_dint_output = (IEC_UDINT *)calloc(elements, sizeof(IEC_UDINT)); - t_lint_input = (IEC_ULINT *)calloc(elements, sizeof(IEC_ULINT)); - t_lint_output = (IEC_ULINT *)calloc(elements, sizeof(IEC_ULINT)); - t_int_memory = (IEC_UINT *)calloc(elements, sizeof(IEC_UINT)); - t_dint_memory = (IEC_UDINT *)calloc(elements, sizeof(IEC_UDINT)); - t_lint_memory = (IEC_ULINT *)calloc(elements, sizeof(IEC_ULINT)); + next.bool_input = (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_INPUT), sizeof(IEC_BOOL *[8])); + next.bool_output = + (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_OUTPUT), sizeof(IEC_BOOL *[8])); + next.bool_memory = + (IEC_BOOL * (*)[8]) calloc(N(IMAGE_TABLE_BOOL_MEMORY), sizeof(IEC_BOOL *[8])); + next.byte_input = (IEC_BYTE **)calloc(N(IMAGE_TABLE_BYTE_INPUT), sizeof(IEC_BYTE *)); + next.byte_output = (IEC_BYTE **)calloc(N(IMAGE_TABLE_BYTE_OUTPUT), sizeof(IEC_BYTE *)); + next.int_input = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_INPUT), sizeof(IEC_UINT *)); + next.int_output = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_OUTPUT), sizeof(IEC_UINT *)); + next.dint_input = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_INPUT), sizeof(IEC_UDINT *)); + next.dint_output = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_OUTPUT), sizeof(IEC_UDINT *)); + next.lint_input = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_INPUT), sizeof(IEC_ULINT *)); + next.lint_output = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_OUTPUT), sizeof(IEC_ULINT *)); + next.int_memory = (IEC_UINT **)calloc(N(IMAGE_TABLE_INT_MEMORY), sizeof(IEC_UINT *)); + next.dint_memory = (IEC_UDINT **)calloc(N(IMAGE_TABLE_DINT_MEMORY), sizeof(IEC_UDINT *)); + next.lint_memory = (IEC_ULINT **)calloc(N(IMAGE_TABLE_LINT_MEMORY), sizeof(IEC_ULINT *)); + + t_bool_input = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_INPUT), sizeof(IEC_BOOL[8])); + t_bool_output = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_OUTPUT), sizeof(IEC_BOOL[8])); + t_bool_memory = (IEC_BOOL(*)[8])calloc(N(IMAGE_TABLE_BOOL_MEMORY), sizeof(IEC_BOOL[8])); + t_byte_input = (IEC_BYTE *)calloc(N(IMAGE_TABLE_BYTE_INPUT), sizeof(IEC_BYTE)); + t_byte_output = (IEC_BYTE *)calloc(N(IMAGE_TABLE_BYTE_OUTPUT), sizeof(IEC_BYTE)); + t_int_input = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_INPUT), sizeof(IEC_UINT)); + t_int_output = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_OUTPUT), sizeof(IEC_UINT)); + t_dint_input = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_INPUT), sizeof(IEC_UDINT)); + t_dint_output = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_OUTPUT), sizeof(IEC_UDINT)); + t_lint_input = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_INPUT), sizeof(IEC_ULINT)); + t_lint_output = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_OUTPUT), sizeof(IEC_ULINT)); + t_int_memory = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_MEMORY), sizeof(IEC_UINT)); + t_dint_memory = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_MEMORY), sizeof(IEC_UDINT)); + t_lint_memory = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_MEMORY), sizeof(IEC_ULINT)); const bool complete = next.bool_input && next.bool_output && next.bool_memory && next.byte_input && next.byte_output && next.int_input && @@ -1185,9 +1229,8 @@ extern "C" bool image_tables_alloc(uint32_t elements) free(t_int_memory); free(t_dint_memory); free(t_lint_memory); - log_error("[image_tables] could not allocate an image of %u elements per table; " - "the previous image is untouched", - elements); + log_error("[image_tables] could not allocate the image; the previous one is untouched"); +#undef N return false; } @@ -1209,36 +1252,95 @@ extern "C" bool image_tables_alloc(uint32_t elements) temp_int_memory = t_int_memory; temp_dint_memory = t_dint_memory; temp_lint_memory = t_lint_memory; - g_capacity = elements; + g_sizes = want; + + /* The SMALLEST table, not the largest, for anything still reading one + * number. Bounding by the smallest refuses an index that would have run + * off the end of it; bounding by the largest reads past every table below + * it. Under-permissive is the only safe direction for a consumer that has + * not been told the tables differ. */ + g_capacity = want.elements[0]; + for (int i = 1; i < IMAGE_TABLE_COUNT; ++i) + { + if (want.elements[i] < g_capacity) + g_capacity = want.elements[i]; + } - log_info("[image_tables] image allocated: %u elements per table", elements); + /* Names only the tables the program actually uses. Fourteen figures of + * which eleven are usually one reads as noise, and the point of the line + * is to let someone watching a load see that the image followed their + * project. */ + { + char summary[256]; + int at = 0; + for (int i = 0; i < IMAGE_TABLE_COUNT && at < (int)sizeof(summary) - 1; ++i) + { + if (want.elements[i] <= IMAGE_MIN_ELEMENTS) + continue; + const int wrote = snprintf(summary + at, sizeof(summary) - (size_t)at, "%s%s=%u", + at ? ", " : "", kImageTableKeys[i], want.elements[i]); + if (wrote < 0 || wrote >= (int)sizeof(summary) - at) + break; + at += wrote; + } + if (at == 0) + snprintf(summary, sizeof(summary), "every table at the minimum"); + log_info("[image_tables] image allocated per table (%s)", summary); + } +#undef N return true; } void image_tables_fill_null_pointers(void) { + /* EACH TABLE WALKED TO ITS OWN LENGTH. + * + * One loop bound for fourteen tables was correct only while they were all + * equal. With per-table sizing the smallest bound would leave the longer + * tables holding null slots -- which is the state a plugin dereferences -- + * and the largest would index past the end of every shorter one, writing + * through a pointer read from beyond the allocation. */ int filled = 0; - for (uint32_t i = 0; i < g_capacity; ++i) - { - for (int b = 0; b < 8; ++b) - { - if (!g_image.bool_input[i][b]) { temp_bool_input[i][b] = 0; g_image.bool_input[i][b] = &temp_bool_input[i][b]; ++filled; } - if (!g_image.bool_output[i][b]) { temp_bool_output[i][b] = 0; g_image.bool_output[i][b] = &temp_bool_output[i][b]; ++filled; } - if (!g_image.bool_memory[i][b]) { temp_bool_memory[i][b] = 0; g_image.bool_memory[i][b] = &temp_bool_memory[i][b]; ++filled; } + +#define FILL_BITS(field, temp, id) \ + for (uint32_t i = 0; i < g_sizes.elements[id]; ++i) \ + for (int b = 0; b < 8; ++b) \ + if (!g_image.field[i][b]) \ + { \ + temp[i][b] = 0; \ + g_image.field[i][b] = &temp[i][b]; \ + ++filled; \ + } + +#define FILL(field, temp, id) \ + for (uint32_t i = 0; i < g_sizes.elements[id]; ++i) \ + if (!g_image.field[i]) \ + { \ + temp[i] = 0; \ + g_image.field[i] = &temp[i]; \ + ++filled; \ } - if (!g_image.byte_input[i]) { temp_byte_input[i] = 0; g_image.byte_input[i] = &temp_byte_input[i]; ++filled; } - if (!g_image.byte_output[i]) { temp_byte_output[i] = 0; g_image.byte_output[i] = &temp_byte_output[i]; ++filled; } - if (!g_image.int_input[i]) { temp_int_input[i] = 0; g_image.int_input[i] = &temp_int_input[i]; ++filled; } - if (!g_image.int_output[i]) { temp_int_output[i] = 0; g_image.int_output[i] = &temp_int_output[i]; ++filled; } - if (!g_image.dint_input[i]) { temp_dint_input[i] = 0; g_image.dint_input[i] = &temp_dint_input[i]; ++filled; } - if (!g_image.dint_output[i]) { temp_dint_output[i] = 0; g_image.dint_output[i] = &temp_dint_output[i]; ++filled; } - if (!g_image.lint_input[i]) { temp_lint_input[i] = 0; g_image.lint_input[i] = &temp_lint_input[i]; ++filled; } - if (!g_image.lint_output[i]) { temp_lint_output[i] = 0; g_image.lint_output[i] = &temp_lint_output[i]; ++filled; } - if (!g_image.int_memory[i]) { temp_int_memory[i] = 0; g_image.int_memory[i] = &temp_int_memory[i]; ++filled; } - if (!g_image.dint_memory[i]) { temp_dint_memory[i] = 0; g_image.dint_memory[i] = &temp_dint_memory[i]; ++filled; } - if (!g_image.lint_memory[i]) { temp_lint_memory[i] = 0; g_image.lint_memory[i] = &temp_lint_memory[i]; ++filled; } - } - log_info("[image_tables] filled %d NULL slots with backing buffers", filled); + + FILL_BITS(bool_input, temp_bool_input, IMAGE_TABLE_BOOL_INPUT) + FILL_BITS(bool_output, temp_bool_output, IMAGE_TABLE_BOOL_OUTPUT) + FILL_BITS(bool_memory, temp_bool_memory, IMAGE_TABLE_BOOL_MEMORY) + FILL(byte_input, temp_byte_input, IMAGE_TABLE_BYTE_INPUT) + FILL(byte_output, temp_byte_output, IMAGE_TABLE_BYTE_OUTPUT) + FILL(int_input, temp_int_input, IMAGE_TABLE_INT_INPUT) + FILL(int_output, temp_int_output, IMAGE_TABLE_INT_OUTPUT) + FILL(dint_input, temp_dint_input, IMAGE_TABLE_DINT_INPUT) + FILL(dint_output, temp_dint_output, IMAGE_TABLE_DINT_OUTPUT) + FILL(lint_input, temp_lint_input, IMAGE_TABLE_LINT_INPUT) + FILL(lint_output, temp_lint_output, IMAGE_TABLE_LINT_OUTPUT) + FILL(int_memory, temp_int_memory, IMAGE_TABLE_INT_MEMORY) + FILL(dint_memory, temp_dint_memory, IMAGE_TABLE_DINT_MEMORY) + FILL(lint_memory, temp_lint_memory, IMAGE_TABLE_LINT_MEMORY) + +#undef FILL_BITS +#undef FILL + + if (filled > 0) + log_info("[image_tables] filled %d null slots with temporaries", filled); } /** @@ -1262,23 +1364,33 @@ static void image_tables_zero_slots(void) // fourteen pointers and leak every table. This is the one function the // static_asserts above point at, and this is the change they were asking // for -- the length comes from g_capacity, never from sizeof. - const uint32_t n = g_capacity; - if (n == 0) return; - - std::memset(g_image.bool_input, 0, (size_t)n * sizeof(IEC_BOOL *[8])); - std::memset(g_image.bool_output, 0, (size_t)n * sizeof(IEC_BOOL *[8])); - std::memset(g_image.bool_memory, 0, (size_t)n * sizeof(IEC_BOOL *[8])); - std::memset(g_image.byte_input, 0, (size_t)n * sizeof(IEC_BYTE *)); - std::memset(g_image.byte_output, 0, (size_t)n * sizeof(IEC_BYTE *)); - std::memset(g_image.int_input, 0, (size_t)n * sizeof(IEC_UINT *)); - std::memset(g_image.int_output, 0, (size_t)n * sizeof(IEC_UINT *)); - std::memset(g_image.dint_input, 0, (size_t)n * sizeof(IEC_UDINT *)); - std::memset(g_image.dint_output, 0, (size_t)n * sizeof(IEC_UDINT *)); - std::memset(g_image.lint_input, 0, (size_t)n * sizeof(IEC_ULINT *)); - std::memset(g_image.lint_output, 0, (size_t)n * sizeof(IEC_ULINT *)); - std::memset(g_image.int_memory, 0, (size_t)n * sizeof(IEC_UINT *)); - std::memset(g_image.dint_memory, 0, (size_t)n * sizeof(IEC_UDINT *)); - std::memset(g_image.lint_memory, 0, (size_t)n * sizeof(IEC_ULINT *)); + /* EACH TABLE BY ITS OWN LENGTH. One figure for fourteen was safe only + * while they were all equal: the smallest would leave the longer tables + * half stale, and the largest would memset past the end of the shorter + * ones -- a heap overflow written by the very function that exists to stop + * this class of mistake. */ + if (image_tables_capacity() == 0 && g_sizes.elements[0] == 0) + return; + +#define Z(field, id, type) \ + if (g_image.field) \ + std::memset(g_image.field, 0, (size_t)g_sizes.elements[id] * sizeof(type)) + + Z(bool_input, IMAGE_TABLE_BOOL_INPUT, IEC_BOOL *[8]); + Z(bool_output, IMAGE_TABLE_BOOL_OUTPUT, IEC_BOOL *[8]); + Z(bool_memory, IMAGE_TABLE_BOOL_MEMORY, IEC_BOOL *[8]); + Z(byte_input, IMAGE_TABLE_BYTE_INPUT, IEC_BYTE *); + Z(byte_output, IMAGE_TABLE_BYTE_OUTPUT, IEC_BYTE *); + Z(int_input, IMAGE_TABLE_INT_INPUT, IEC_UINT *); + Z(int_output, IMAGE_TABLE_INT_OUTPUT, IEC_UINT *); + Z(dint_input, IMAGE_TABLE_DINT_INPUT, IEC_UDINT *); + Z(dint_output, IMAGE_TABLE_DINT_OUTPUT, IEC_UDINT *); + Z(lint_input, IMAGE_TABLE_LINT_INPUT, IEC_ULINT *); + Z(lint_output, IMAGE_TABLE_LINT_OUTPUT, IEC_ULINT *); + Z(int_memory, IMAGE_TABLE_INT_MEMORY, IEC_UINT *); + Z(dint_memory, IMAGE_TABLE_DINT_MEMORY, IEC_UDINT *); + Z(lint_memory, IMAGE_TABLE_LINT_MEMORY, IEC_ULINT *); +#undef Z } void image_tables_clear_null_pointers(void) diff --git a/core/src/plc_app/image_tables.h b/core/src/plc_app/image_tables.h index 2e520672..3a5d9425 100644 --- a/core/src/plc_app/image_tables.h +++ b/core/src/plc_app/image_tables.h @@ -1,6 +1,7 @@ #ifndef IMAGE_TABLES_H #define IMAGE_TABLES_H +#include "image_table_id.h" #include #include #include @@ -114,27 +115,8 @@ extern "C" * or whose editor predates the file, still comes up correct. * --------------------------------------------------------------------- */ - /* One id per table, in the order image_tables_t declares them. Note the gap - * the list makes visible: byte_input and byte_output exist, byte_memory - * does not, so `%MB` has no storage on this runtime at all. */ - typedef enum - { - IMAGE_TABLE_BOOL_INPUT = 0, - IMAGE_TABLE_BOOL_OUTPUT, - IMAGE_TABLE_BYTE_INPUT, - IMAGE_TABLE_BYTE_OUTPUT, - IMAGE_TABLE_INT_INPUT, - IMAGE_TABLE_INT_OUTPUT, - IMAGE_TABLE_DINT_INPUT, - IMAGE_TABLE_DINT_OUTPUT, - IMAGE_TABLE_LINT_INPUT, - IMAGE_TABLE_LINT_OUTPUT, - IMAGE_TABLE_INT_MEMORY, - IMAGE_TABLE_DINT_MEMORY, - IMAGE_TABLE_LINT_MEMORY, - IMAGE_TABLE_BOOL_MEMORY, - IMAGE_TABLE_COUNT - } image_table_id_t; + /* The table identities live in their own header so plugin_types.h can + * reach them without pulling the runtime internals in. */ /* Elements per table, in that table's own unit -- which for the three BOOL * tables is BYTES, because they are declared [N][8], and for every other @@ -167,47 +149,19 @@ extern "C" void image_sizes_take_max(image_sizes_t *dst, const image_sizes_t *other); /** - * The single element count the whole image is allocated at. - * - * ONE NUMBER FOR FOURTEEN TABLES, and the reason is the plugin ABI rather - * than convenience. `plugin_runtime_args_t` carries a single `buffer_size` - * (plugin_types.h), and plugins bounds-check against it -- ethercat_io.c - * refuses a byte_index at or above it, s7comm derives every clamp from it. - * That works today only because the fourteen tables happen to be the same - * size, so one number describes them all. - * - * Give each table its own size and no value of that field is correct: the - * minimum makes every plugin refuse everything the moment one table is - * empty (a project with `%QW4096` and no `%IX` would have a floor of zero), - * and the maximum lets a plugin write past the end of the smaller tables -- - * the exact overflow this work exists to prevent. Per-table sizes would - * 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: every table allocated at the largest count any of - * them needs. The `image.conf` still carries all fourteen numbers, because - * bare metal DOES size each area independently -- it has no plugin ABI to - * satisfy, and each `MAX_*` there dimensions its own array. Only Runtime v4 - * collapses them, and the file is ready if that ever stops being true. + * Make every table the same length: the largest any of them needs. * - * The cost, counted properly: a program needing 4096 output words gets 4096 - * elements in all fourteen tables. On a 64-bit target the three BOOL tables - * are `IEC_BOOL *[8]`, so 64 bytes per element rather than 8 -- 786 KB -- - * the other eleven add 360 KB, and the `temp_*` backing buffers are sized - * at `elements` too and add about 272 KB. Roughly **1.36 MiB**. + * The square fallback, for a run where some plugin does not understand + * per-table sizes. It takes and returns an `image_sizes_t` rather than + * collapsing to one number on purpose (RTOP-284): a single figure for + * fourteen tables is the assumption this work exists to remove, and a + * helper that produces one is an invitation to reintroduce it. * - * (An earlier version of this comment said ~460 KB. It counted eight bytes - * per BOOL element instead of sixty-four and left the backing buffers out - * entirely, which understated the figure about threefold. The number is - * what carries the square-image decision over per-table sizing, so it is - * worth having right: 1.36 MiB on a Linux target is still small against - * breaking every pre-compiled plugin, but it is not 460 KB.) - * - * The gain the demand asked for is untouched -- 240 I/O points stop hitting - * a ceiling of 1024, and a small project stops paying for 1024 of - * everything. + * The LARGEST, not the smallest, because square has to cover every + * area the program actually uses. It costs memory the project does not + * need, which is the price of a plugin that cannot be told the truth. */ - uint32_t image_sizes_largest(const image_sizes_t *sizes); + void image_sizes_flatten(image_sizes_t *sizes); /** The most any one table may hold: the ceiling of the uint16 `byte_index` * in the STruC++ ABI, so no located variable can address beyond it. The @@ -241,7 +195,7 @@ extern "C" * stops. A partial image would be worse than none, because every table * indexes the same way whether it is real or null. */ - bool image_tables_alloc(uint32_t elements); + bool image_tables_alloc(const image_sizes_t *sizes); /** Release the image. Safe to call when nothing is allocated. * CALLER MUST HOLD THE IMAGE-TABLES MUTEX. */ @@ -250,6 +204,19 @@ extern "C" /** How many elements each table currently holds; 0 before any allocation. */ uint32_t image_tables_capacity(void); + /** How long one table actually is, in its own elements. + * + * The tables no longer share a length, so this is the only honest answer + * to "how far does this area reach". `image_tables_capacity()` remains for + * consumers that read a single number and returns the SMALLEST of the + * fourteen, which refuses an index rather than letting one run off the end + * of a shorter table. + * + * Zero for an id outside the enum, which is the safe reading: a caller + * that asks about a table this runtime does not have gets an area it + * cannot index into. */ + uint32_t image_table_capacity(image_table_id_t id); + /* ------------------------------------------------------------------------- * Resolved .so symbols (populated by symbols_init). * diff --git a/core/src/plc_app/journal_buffer.c b/core/src/plc_app/journal_buffer.c index e1f47d5b..d971338d 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -23,6 +23,7 @@ */ #include "journal_buffer.h" +#include "image_tables.h" #include "utils/log.h" #include "utils/utils.h" #include @@ -82,6 +83,58 @@ static int journal_add(uint8_t type, uint16_t index, uint8_t bit, uint64_t value * never been made. The image can be any size now, so this follows it: one row * per journal type, each as long as the image. * --------------------------------------------------------------------------- */ +/* JOURNAL TYPES AND IMAGE TABLE IDS ARE NOT THE SAME ORDER, despite both + * having fourteen members and journal_buffer.h saying this enum "matches the + * OpenPLC image table types". It matches the CONCEPTS, not the indices: + * journal puts each width's memory table beside its input and output + * (..._INPUT, ..._OUTPUT, ..._MEMORY) while image_tables.h groups all the + * memory tables at the end. JOURNAL_INT_MEMORY is 7; IMAGE_TABLE_INT_MEMORY + * is 10. + * + * So a cast between them is silent corruption: writes land in another table's + * bounds and the wrong area is refused or admitted. The mapping is written + * out, once, here. */ +static const image_table_id_t kJournalToImageTable[JOURNAL_TYPE_COUNT] = { + [JOURNAL_BOOL_INPUT] = IMAGE_TABLE_BOOL_INPUT, + [JOURNAL_BOOL_OUTPUT] = IMAGE_TABLE_BOOL_OUTPUT, + [JOURNAL_BOOL_MEMORY] = IMAGE_TABLE_BOOL_MEMORY, + [JOURNAL_BYTE_INPUT] = IMAGE_TABLE_BYTE_INPUT, + [JOURNAL_BYTE_OUTPUT] = IMAGE_TABLE_BYTE_OUTPUT, + [JOURNAL_INT_INPUT] = IMAGE_TABLE_INT_INPUT, + [JOURNAL_INT_OUTPUT] = IMAGE_TABLE_INT_OUTPUT, + [JOURNAL_INT_MEMORY] = IMAGE_TABLE_INT_MEMORY, + [JOURNAL_DINT_INPUT] = IMAGE_TABLE_DINT_INPUT, + [JOURNAL_DINT_OUTPUT] = IMAGE_TABLE_DINT_OUTPUT, + [JOURNAL_DINT_MEMORY] = IMAGE_TABLE_DINT_MEMORY, + [JOURNAL_LINT_INPUT] = IMAGE_TABLE_LINT_INPUT, + [JOURNAL_LINT_OUTPUT] = IMAGE_TABLE_LINT_OUTPUT, + [JOURNAL_LINT_MEMORY] = IMAGE_TABLE_LINT_MEMORY, +}; + +/** The longest table, which is how long a forced-slot row has to be: rows are + * one length for all fourteen types, so the longest is the only one that can + * record a forced slot anywhere any table reaches. Under-allocating here is + * what silently stopped a high address being forced at all. */ +static uint32_t journal_longest_table(void) +{ + uint32_t longest = 0; + for (int t = 0; t < JOURNAL_TYPE_COUNT; ++t) + { + const uint32_t n = image_table_capacity(kJournalToImageTable[t]); + if (n > longest) + longest = n; + } + return longest; +} + +/** How far this journal type's table actually reaches. */ +static uint32_t journal_type_capacity(uint8_t type) +{ + if (type >= JOURNAL_TYPE_COUNT) + return 0; + return image_table_capacity(kJournalToImageTable[type]); +} + static uint8_t *g_forced[JOURNAL_TYPE_COUNT]; /* uint32_t, not uint16_t: the image is allowed up to 65536 elements, which does * not fit a uint16_t and would wrap to zero -- turning "the largest legal @@ -142,7 +195,14 @@ static inline int is_slot_forced(uint8_t type, uint16_t idx, uint8_t bit) { if (g_force_count == 0) return 0; /* fast path: nothing forced */ - if (type >= JOURNAL_TYPE_COUNT || idx >= g_force_size) + /* Two bounds, and both matter: the row has to exist (g_force_size is how + * long every row was allocated) and the slot has to be one this table + * actually has. With the tables at different lengths the second is the + * real one -- a row is as long as the LARGEST table so every type has + * somewhere to record, and the per-table check is what stops a forced slot + * being honoured in an area that does not reach that far. */ + if (type >= JOURNAL_TYPE_COUNT || idx >= g_force_size || + (uint32_t)idx >= journal_type_capacity(type)) return 0; if (type_is_bool(type)) { @@ -159,13 +219,17 @@ static void apply_write_raw(const journal_entry_t *entry) { uint16_t idx = entry->index; - /* Bounds check. Compared as a signed int rather than through a - * (uint16_t) cast: buffer_size is an int and the image may reach 65536, - * which that cast turns into 0 -- dropping EVERY journal write with no - * diagnostic, at exactly the largest legal image. It is the same wrap the - * comment above g_force_size describes, and this was the one site the - * widening there missed. `idx` is uint16_t and promotes cleanly. */ - if ((int)idx >= g_buffer_ptrs.buffer_size) + /* Bounds check against THIS ENTRY'S OWN TABLE. + * + * It used to compare against g_buffer_ptrs.buffer_size, one figure for + * fourteen tables. That number is now the SMALLEST of them, so a write to + * any longer table above the smallest table's length would be dropped -- + * silently, which is the failure mode this whole area keeps producing. + * + * Still compared as uint32_t rather than through a (uint16_t) cast: the + * image may reach 65536, which that cast turns into 0 and drops every + * write at exactly the largest legal image. */ + if ((uint32_t)idx >= journal_type_capacity(entry->buffer_type)) { return; } @@ -458,13 +522,13 @@ int journal_init(const journal_buffer_ptrs_t *buffer_ptrs) memcpy(&g_buffer_ptrs, buffer_ptrs, sizeof(journal_buffer_ptrs_t)); /* The forced-slot bitmap follows the image, so forcing works across the - * whole of it rather than the first 1024 slots. buffer_size comes from - * image_tables_capacity(), set when the image was allocated for this - * program. */ - if (force_map_alloc((uint32_t)g_buffer_ptrs.buffer_size) != 0) + * whole of it rather than the first 1024 slots. Rows are as long as the + * LONGEST table, because they are one length for all fourteen types and + * anything shorter cannot record a forced slot in the tables above it. */ + if (force_map_alloc(journal_longest_table()) != 0) { - log_error("Journal: could not allocate the forced-slot map for %d slots", - g_buffer_ptrs.buffer_size); + log_error("Journal: could not allocate the forced-slot map for %u slots", + journal_longest_table()); return -1; } @@ -649,10 +713,10 @@ int journal_init(const journal_buffer_ptrs_t *buffer_ptrs) * journal_apply_and_clear and journal_is_initialized would block, taking * the scan thread with them, and journal_cleanup could not recover it. The * map depends on nothing this lock protects. */ - if (force_map_alloc((uint32_t)buffer_ptrs->buffer_size) != 0) + if (force_map_alloc(journal_longest_table()) != 0) { - log_error("Journal: could not allocate the forced-slot map for %d slots", - buffer_ptrs->buffer_size); + log_error("Journal: could not allocate the forced-slot map for %u slots", + journal_longest_table()); return -1; } diff --git a/core/src/plc_app/journal_buffer.h b/core/src/plc_app/journal_buffer.h index 8ce684b4..635c66b2 100644 --- a/core/src/plc_app/journal_buffer.h +++ b/core/src/plc_app/journal_buffer.h @@ -115,7 +115,16 @@ typedef struct { IEC_ULINT **lint_output; IEC_ULINT **lint_memory; - /* Buffer size (number of elements in each array) */ + /* THE SMALLEST ARRAY, NOT THE LENGTH OF ALL OF THEM (RTOP-284). + * + * The arrays above no longer share a length. This field was a second copy + * of the one-number assumption, internal to the runtime, and it is kept + * only for the few places that still want a conservative single figure: + * it is the minimum, so using it as a bound refuses an index rather than + * letting one run off the end of a shorter array. + * + * Anything bounding a WRITE asks image_table_capacity() for the table that + * write is going to, which is what apply_write_raw does. */ int buffer_size; /* Image table mutex (for emergency flush and apply operations) */ diff --git a/core/src/plc_app/plc_main.c b/core/src/plc_app/plc_main.c index bf5841c3..c4c9a9d3 100644 --- a/core/src/plc_app/plc_main.c +++ b/core/src/plc_app/plc_main.c @@ -155,11 +155,14 @@ int main(int argc, char *argv[]) * buffer_size into every plugin's args, and a plugin is entitled to * a valid image from the moment it initialises -- never a NULL base * pointer and never a zero size. The minimum is what - * image_tables_alloc() clamps to: the smallest count that is not no - * image at all. A program load reallocates it properly. */ + * image_tables_alloc() clamps to, per table: the smallest count + * that is not no image at all. NULL asks for exactly that, which + * says "no program has told me anything yet" rather than passing a + * zeroed struct that reads like a real answer. A program load + * reallocates it properly. */ pthread_mutex_t *itm = image_tables_mutex(); pthread_mutex_lock(itm); - const bool image_ok = image_tables_alloc(0); + const bool image_ok = image_tables_alloc(NULL); pthread_mutex_unlock(itm); if (!image_ok) diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index 5dc393d7..241b793f 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -1080,9 +1080,30 @@ extern "C" int load_plc_program(PluginManager *pm) image_sizes_derive_floor(pm, &floor); image_sizes_take_max(&configured, &floor); + /* PER TABLE, OR SQUARE, DECIDED PER RUN. + * + * Per-table is what the project asked for and what the image + * exists to deliver. It is only safe when every loaded plugin + * understands it: 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. + * One that does not export set_image_sizes has not been told + * they can differ, so for that run they do not. + * + * Logged with the plugin that forced it, because the two modes + * are otherwise indistinguishable from outside. */ + const char *forced_by = NULL; + if (!plugin_driver_all_understand_per_table_sizes(plugin_driver, &forced_by)) + { + image_sizes_flatten(&configured); + log_info("[PLUGIN]: image kept square: plugin '%s' does not declare " + "set_image_sizes", + forced_by ? forced_by : "(unknown)"); + } + pthread_mutex_t *itm = image_tables_mutex(); pthread_mutex_lock(itm); - const bool ok = image_tables_alloc(image_sizes_largest(&configured)); + const bool ok = image_tables_alloc(&configured); pthread_mutex_unlock(itm); if (!ok) diff --git a/tests/pytest/test_image_conf_contract.py b/tests/pytest/test_image_conf_contract.py index 88b93fad..092742b7 100644 --- a/tests/pytest/test_image_conf_contract.py +++ b/tests/pytest/test_image_conf_contract.py @@ -40,15 +40,23 @@ # this test was written to be. REPO_ROOT = Path(__file__).resolve().parents[2] IMAGE_TABLES_H = REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.h" +# The enum moved out of image_tables.h so plugin_types.h could reach it without +# pulling the runtime internals in (RTOP-284, B2). Publishing a type costs no +# ABI, and a plugin receiving an array indexed by it has to be able to name the +# entries -- the alternative being a second copy of the enum, which is the +# drift this whole file exists to catch. +IMAGE_TABLE_ID_H = REPO_ROOT / "core" / "src" / "plc_app" / "image_table_id.h" IMAGE_TABLES_CPP = REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.cpp" +JOURNAL_H = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.h" +JOURNAL_C = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.c" def _enum_ids() -> list[str]: """`image_table_id_t` members, in declaration order, lowercased.""" body = re.search( - r"typedef enum\s*\{(.*?)\}\s*image_table_id_t", IMAGE_TABLES_H.read_text(), re.S + r"typedef enum\s*\{(.*?)\}\s*image_table_id_t", IMAGE_TABLE_ID_H.read_text(), re.S ) - assert body, "image_table_id_t not found — has the header been restructured?" + assert body, "image_table_id_t not found — has image_table_id.h been restructured?" return [m.lower() for m in re.findall(r"IMAGE_TABLE_([A-Z_]+)", body.group(1)) if m != "COUNT"] @@ -160,3 +168,63 @@ def test_the_abi_limit_matches_the_index_width(): # is where this number comes from. It is a fact of the ABI, not a policy # ceiling, so it moves only if that field does. assert image_config.MAX_TABLE_ELEMENTS == 1 << 16 + + +class TestJournalMapping: + """The journal's type enum and the image table enum are NOT the same order. + + Both have fourteen members and journal_buffer.h says this enum "matches the + OpenPLC image table types" -- it matches the concepts, not the indices. + The journal groups each width's memory table beside its input and output; + image_tables.h puts every memory table at the end. JOURNAL_INT_MEMORY is 7 + and IMAGE_TABLE_INT_MEMORY is 10. + + A cast between them therefore corrupts silently: a write lands under + another table's bounds, and an area is refused or admitted wrongly. The + runtime maps them explicitly (kJournalToImageTable); this checks that the + map is complete and that it is still needed. + """ + + @staticmethod + def _journal_ids() -> list[str]: + body = re.search( + r"typedef enum\s*\{(.*?)\}\s*journal_buffer_type_t", JOURNAL_H.read_text(), re.DOTALL + ) + assert body, "journal_buffer_type_t not found" + return [ + m.lower() for m in re.findall(r"JOURNAL_([A-Z_]+)", body.group(1)) if m != "TYPE_COUNT" + ] + + @staticmethod + def _mapping() -> dict[str, str]: + body = re.search( + r"kJournalToImageTable\[JOURNAL_TYPE_COUNT\] = \{(.*?)\};", + JOURNAL_C.read_text(), + re.DOTALL, + ) + assert body, "kJournalToImageTable not found — has the journal been restructured?" + return { + journal.lower(): image.lower() + for journal, image in re.findall( + r"\[JOURNAL_([A-Z_]+)\]\s*=\s*IMAGE_TABLE_([A-Z_]+)", body.group(1) + ) + } + + def test_every_journal_type_maps_to_a_table(self): + assert sorted(self._mapping()) == sorted(self._journal_ids()) + + def test_each_one_maps_to_the_table_of_the_same_name(self): + # The map is about ORDER, not renaming: JOURNAL_INT_MEMORY must reach + # IMAGE_TABLE_INT_MEMORY, whatever index either one sits at. + for journal, image in self._mapping().items(): + assert journal == image, f"JOURNAL_{journal.upper()} maps to the wrong table" + + def test_the_two_enums_really_do_disagree_on_order(self): + # If they were ever made identical the map could go -- but silently + # assuming they are identical is the bug. This fails if someone + # reorders one to match, which is the moment to revisit the map on + # purpose rather than discover it by corruption. + assert self._journal_ids() != list(image_config.IMAGE_TABLE_KEYS) + + def test_the_journal_covers_every_table_the_image_has(self): + assert sorted(self._journal_ids()) == sorted(image_config.IMAGE_TABLE_KEYS) diff --git a/tests/pytest/test_modbus_exposure_fit.py b/tests/pytest/test_modbus_exposure_fit.py index 532beeb4..59385444 100644 --- a/tests/pytest/test_modbus_exposure_fit.py +++ b/tests/pytest/test_modbus_exposure_fit.py @@ -232,3 +232,60 @@ def test_the_legacy_shape_has_no_memory_segments(sm): assert parsed["holding_registers"]["mw_count"] == 0 assert parsed["coils"]["mx_bits"] == 0 assert parsed["word_order"] == "high_word_first" + + +# --- per-table clamping (RTOP-284, C1.3/C1.4) ---------------------------- + + +@pytest.fixture +def per_table(sm): + """Deliver per-table sizes the way the runtime does, then clear them.""" + from shared import image_sizes + + def deliver(**by_name): + sizes = [by_name.get(name, 0) for name in image_sizes.IMAGE_TABLE_ORDER] + assert image_sizes.set_image_sizes(sizes) == 0 + + yield deliver + image_sizes.set_image_sizes([]) + + +def test_each_segment_is_clamped_against_its_own_table(sm, per_table): + # %QW comes out of int_output and %MW out of int_memory. One figure for + # both would have to be the smaller, losing the larger table's range. + per_table(int_output=100, int_memory=4) + parsed = sm.parse_buffer_mapping_config(segmented(qw=100, mw=100), 8) + assert parsed["holding_registers"]["qw_count"] == 100 + assert parsed["holding_registers"]["mw_count"] == 4 + + +def test_a_bit_segment_reads_its_table_in_bits(sm, per_table): + # bool_output is in elements of eight; %QX addresses the bits. + per_table(bool_output=2) + parsed = sm.parse_buffer_mapping_config(segmented(qx=8192), 8) + assert parsed["coils"]["qx_bits"] == 16 + + +def test_an_empty_table_exposes_nothing(sm, per_table): + per_table(int_output=0, int_memory=50) + parsed = sm.parse_buffer_mapping_config(segmented(qw=10, mw=10), 8) + assert parsed["holding_registers"]["qw_count"] == 0 + assert parsed["holding_registers"]["mw_count"] == 10 + + +def test_a_square_run_falls_back_to_the_single_figure(sm): + # No sizes delivered: buffer_size IS the length every table has, so it is + # the right answer rather than a guess. + from shared import image_sizes + + image_sizes.set_image_sizes([]) + parsed = sm.parse_buffer_mapping_config(segmented(qw=100, mw=100), 8) + assert parsed["holding_registers"]["qw_count"] == 8 + assert parsed["holding_registers"]["mw_count"] == 8 + + +def test_the_module_exports_the_symbol_the_runtime_looks_for(sm): + # PyObject_GetAttrString(pModule, "set_image_sizes") has to find it HERE, + # in this module's namespace -- importing it is what declares the + # capability, and without it every run this plugin is in stays square. + assert callable(getattr(sm, "set_image_sizes", None)) From 41c202a28069e33145d2a56be2225f066e6128a2 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 09:12:59 -0300 Subject: [PATCH 2/5] feat(image): let plugin_types.h name the image tables 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 --- core/src/drivers/plugin_types.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index 3b9d7a02..a7b34dee 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -16,6 +16,16 @@ #define PLUGIN_TYPES_H #include "../lib/iec_types.h" +/* The image table identities, so a plugin receiving the sizes array can name + * the entries it indexes rather than counting positions (RTOP-284, B2). + * Publishing a type costs no ABI: no struct gains a field and no offset + * moves, which is what CON06 guarantees pre-compiled plugins. + * + * A plugin built against an OLDER runtime will not find this header, so + * anything that must work on both — a VPP package, which ships and is built + * independently of the runtime on the device — carries its own constants and + * keeps them in the order documented there. */ +#include "../plc_app/image_table_id.h" #include #include #include From 1e6a18cef4514a9e1710d0b93f99c86f70214e90 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 17:07:20 -0300 Subject: [PATCH 3/5] fix(image): make the feature actually engage, and pin the three loose 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 --- core/src/drivers/plugin_driver.c | 15 +++ .../modbus_master/modbus_master_plugin.py | 74 +++++++++---- .../drivers/plugins/python/opcua/plugin.py | 12 +++ core/src/plc_app/journal_buffer.c | 32 +++++- tests/pytest/test_image_conf_contract.py | 101 +++++++++++++++++- .../test_plugins_declare_image_sizes.py | 81 ++++++++++++++ 6 files changed, 286 insertions(+), 29 deletions(-) create mode 100644 tests/pytest/test_plugins_declare_image_sizes.py diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 32b92288..ae8aa5d5 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -677,6 +677,21 @@ bool plugin_driver_all_understand_per_table_sizes(plugin_driver_t *driver, plugin_instance_t *plugin = &driver->plugins[i]; bool understands = false; + /* A DEGRADED PLUGIN DOES NOT GET 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" would mean one box missing Npcap, where the EtherCAT + * plugin degrades, silently costs every OTHER plugin its per-table + * image -- a downgrade with no relation to anything that will actually + * read the tables. + * + * DISABLED plugins still vote, deliberately: plugin_driver_init + * initialises them regardless of the enabled flag, so a disabled + * plugin does hold the base pointers and does read the image. */ + if (plugin->degraded) + continue; + if (plugin->config.type == PLUGIN_TYPE_NATIVE) understands = plugin->native_plugin && plugin->native_plugin->set_image_sizes; else if (plugin->config.type == PLUGIN_TYPE_PYTHON) diff --git a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py index e9d636be..da601808 100644 --- a/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py +++ b/core/src/drivers/plugins/python/modbus_master/modbus_master_plugin.py @@ -20,6 +20,17 @@ ) # Import the configuration model +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). The runtime keeps +# the image SQUARE for any run in which even one loaded plugin lacks it -- and +# this plugin ships in plugins_default.conf, so without this line per-table +# sizing never activates on a stock device. +# +# Nothing else is needed here: this plugin bounds through SafeBufferAccess -> +# BufferValidator, which already validates against the table each buffer lives +# in rather than against the single figure. +from shared.image_sizes import set_image_sizes # noqa: F401 from shared.plugin_config_decode.modbus_master_config_model import ( ERROR_HANDLING_SET_TO_ZERO, ModbusMasterConfig, @@ -97,6 +108,7 @@ class ModbusSlaveDevice(threading.Thread): Handles a single Modbus TCP device with its own connection. For RTU devices, use ModbusRtuBusHandler instead. """ + def __init__(self, device_config: Any, sba: SafeBufferAccess, plugin_logger: PluginLogger): super().__init__(daemon=True) self.device_config = device_config @@ -110,9 +122,11 @@ def __init__(self, device_config: Any, sba: SafeBufferAccess, plugin_logger: Plu host=device_config.host, port=device_config.port, timeout_ms=device_config.timeout_ms, - slave_id=device_config.slave_id + slave_id=device_config.slave_id, + ) + self.name = ( + f"ModbusSlave-{device_config.name}-TCP-{device_config.host}:{device_config.port}" ) - self.name = f"ModbusSlave-{device_config.name}-TCP-{device_config.host}:{device_config.port}" # Calculate GCD of all I/O point cycle times for this device self.gcd_cycle_time_ms = calculate_gcd_of_cycle_times(device_config.io_points) @@ -139,7 +153,9 @@ def run(self): # pylint: disable=too-many-locals # Connect with infinite retry if not self.connection_manager.connect_with_retry(self._stop_event): - self.logger.info(f"[{self.name}] Thread stopped before connection could be established.") + self.logger.info( + f"[{self.name}] Thread stopped before connection could be established." + ) return # Initialize cycle counter @@ -368,23 +384,25 @@ def run(self): # pylint: disable=too-many-locals if point.fc == 5: # Write Single Coil if len(values_to_write) > 0: response = self.connection_manager.client.write_coil( - address, values_to_write[0], device_id=self.connection_manager.slave_id + address, + values_to_write[0], + device_id=self.connection_manager.slave_id, ) else: self.logger.error( - f"[{self.name}] No data to write " - f"for FC 5, offset {address}" + f"[{self.name}] No data to write " f"for FC 5, offset {address}" ) continue elif point.fc == 6: # Write Single Register if len(values_to_write) > 0: response = self.connection_manager.client.write_register( - address, values_to_write[0], device_id=self.connection_manager.slave_id + address, + values_to_write[0], + device_id=self.connection_manager.slave_id, ) else: self.logger.error( - f"[{self.name}] No data to write " - f"for FC 6, offset {address}" + f"[{self.name}] No data to write " f"for FC 6, offset {address}" ) continue elif point.fc == 15: # Write Multiple Coils @@ -478,10 +496,10 @@ class ModbusBusHandler(threading.Thread): def __init__( self, - transport: str, # "tcp" or "rtu" - connection_config: dict, # tcp: {host, port, timeout_ms}; - # rtu: {serial_port, baud_rate, parity, stop_bits, data_bits, timeout_ms} - devices: List[Any], # List of ModbusDeviceConfig sharing this connection + transport: str, # "tcp" or "rtu" + connection_config: dict, # tcp: {host, port, timeout_ms}; + # rtu: {serial_port, baud_rate, parity, stop_bits, data_bits, timeout_ms} + devices: List[Any], # List of ModbusDeviceConfig sharing this connection sba: SafeBufferAccess, plugin_logger: PluginLogger, ): @@ -522,17 +540,23 @@ def __init__( self.all_io_points = [] for device in devices: for point in device.io_points: - self.all_io_points.append({ - "point": point, - "slave_id": device.slave_id, - "device_name": device.name, - }) + self.all_io_points.append( + { + "point": point, + "slave_id": device.slave_id, + "device_name": device.name, + } + ) # Calculate GCD of all IO point cycle times across all devices on this bus all_cycle_times = [p.cycle_time_ms for d in devices for p in d.io_points] - self.gcd_cycle_time_ms = calculate_gcd_of_cycle_times( - [type('obj', (object,), {'cycle_time_ms': ct})() for ct in all_cycle_times] - ) if all_cycle_times else 1000 + self.gcd_cycle_time_ms = ( + calculate_gcd_of_cycle_times( + [type("obj", (object,), {"cycle_time_ms": ct})() for ct in all_cycle_times] + ) + if all_cycle_times + else 1000 + ) device_names = ", ".join([d.name for d in devices]) self.logger.info( @@ -555,7 +579,9 @@ def run(self): # pylint: disable=too-many-locals,too-many-branches,too-many-sta # Connect with infinite retry if not self.connection_manager.connect_with_retry(self._stop_event): - self.logger.info(f"[{self.name}] Thread stopped before connection could be established.") + self.logger.info( + f"[{self.name}] Thread stopped before connection could be established." + ) return # Initialize cycle counter @@ -1047,7 +1073,9 @@ def start_loop(): try: if len(endpoint_devices) == 1: device_config = endpoint_devices[0] - device_thread = ModbusSlaveDevice(device_config, safe_buffer_accessor, logger) + device_thread = ModbusSlaveDevice( + device_config, safe_buffer_accessor, logger + ) device_thread.start() slave_threads.append(device_thread) logger.info( diff --git a/core/src/drivers/plugins/python/opcua/plugin.py b/core/src/drivers/plugins/python/opcua/plugin.py index 084c76b7..0c551fdf 100644 --- a/core/src/drivers/plugins/python/opcua/plugin.py +++ b/core/src/drivers/plugins/python/opcua/plugin.py @@ -32,6 +32,18 @@ SafeLoggingAccess, safe_extract_runtime_args_from_capsule, ) + +# Importing set_image_sizes is not a formality: the name has to exist in THIS +# module for the runtime to find it, and its presence is how this plugin +# declares it understands per-table image sizes (RTOP-284). The runtime keeps +# the image SQUARE for any run in which even one loaded plugin lacks it -- and +# this plugin ships in plugins_default.conf, so without this line per-table +# sizing never activates on a stock device. +# +# Nothing else is needed here: this plugin bounds through SafeBufferAccess -> +# BufferValidator, which already validates against the table each buffer lives +# in rather than against the single figure. +from shared.image_sizes import set_image_sizes # noqa: F401 from shared.plugin_config_decode.opcua_config_model import OpcuaConfig # Import local modules (use absolute imports for runtime compatibility) diff --git a/core/src/plc_app/journal_buffer.c b/core/src/plc_app/journal_buffer.c index b92ba8fe..7f0e3600 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -447,7 +447,21 @@ static void apply_entry(const journal_entry_t *entry) * under image_lock — the same serialization domain as apply_entry. */ void journal_force_set(journal_buffer_type_t type, uint16_t index, uint8_t bit, uint64_t value) { - if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size) + /* BOTH BOUNDS: the row AND the table. + * + * g_force_size is how long every row was allocated -- the LONGEST table, + * so each type has somewhere to record. It is not how far this type's + * table reaches. While every table had the same length the two were one + * number and could not disagree; they can now. + * + * With bool_output at 1 element and int_output at 100, g_force_size is + * 100, so forcing bool_output index 5 passed this check, flipped the bit + * and incremented g_force_count -- permanently disabling the fast path in + * is_slot_forced -- while apply_write_raw and is_slot_forced both refused + * it on the per-table bound. A force that did nothing at all, and said + * nothing, which is the failure this guard exists to report. */ + if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size || + (uint32_t)index >= journal_type_capacity((uint8_t)type)) { /* Counted, not logged: see g_force_oob_drops. When the map was never * allocated g_force_size is 0 and EVERY force lands here. */ @@ -477,7 +491,21 @@ void journal_force_set(journal_buffer_type_t type, uint16_t index, uint8_t bit, * plugin) is no longer dropped, so the slot tracks the live value again. */ void journal_force_clear(journal_buffer_type_t type, uint16_t index, uint8_t bit) { - if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size) + /* BOTH BOUNDS: the row AND the table. + * + * g_force_size is how long every row was allocated -- the LONGEST table, + * so each type has somewhere to record. It is not how far this type's + * table reaches. While every table had the same length the two were one + * number and could not disagree; they can now. + * + * With bool_output at 1 element and int_output at 100, g_force_size is + * 100, so forcing bool_output index 5 passed this check, flipped the bit + * and incremented g_force_count -- permanently disabling the fast path in + * is_slot_forced -- while apply_write_raw and is_slot_forced both refused + * it on the per-table bound. A force that did nothing at all, and said + * nothing, which is the failure this guard exists to report. */ + if ((uint8_t)type >= JOURNAL_TYPE_COUNT || index >= g_force_size || + (uint32_t)index >= journal_type_capacity((uint8_t)type)) { /* Counted, not logged: see g_force_oob_drops. When the map was never * allocated g_force_size is 0 and EVERY force lands here. */ diff --git a/tests/pytest/test_image_conf_contract.py b/tests/pytest/test_image_conf_contract.py index 092742b7..a3239013 100644 --- a/tests/pytest/test_image_conf_contract.py +++ b/tests/pytest/test_image_conf_contract.py @@ -49,12 +49,15 @@ IMAGE_TABLES_CPP = REPO_ROOT / "core" / "src" / "plc_app" / "image_tables.cpp" JOURNAL_H = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.h" JOURNAL_C = REPO_ROOT / "core" / "src" / "plc_app" / "journal_buffer.c" +S7COMM_C = REPO_ROOT / "core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp" +ETHERCAT_C = REPO_ROOT / "core/src/drivers/plugins/native/ethercat/ethercat_io.c" +MODBUS_PY = REPO_ROOT / "core/src/drivers/plugins/python/modbus_slave/simple_modbus.py" def _enum_ids() -> list[str]: """`image_table_id_t` members, in declaration order, lowercased.""" body = re.search( - r"typedef enum\s*\{(.*?)\}\s*image_table_id_t", IMAGE_TABLE_ID_H.read_text(), re.S + r"typedef enum\s*\{(.*?)\}\s*image_table_id_t", IMAGE_TABLE_ID_H.read_text(), re.DOTALL ) assert body, "image_table_id_t not found — has image_table_id.h been restructured?" return [m.lower() for m in re.findall(r"IMAGE_TABLE_([A-Z_]+)", body.group(1)) if m != "COUNT"] @@ -63,7 +66,7 @@ def _enum_ids() -> list[str]: def _c_keys() -> list[str]: """The strings `kImageTableKeys` maps those ids to, in order.""" body = re.search( - r"kImageTableKeys\[IMAGE_TABLE_COUNT\] = \{(.*?)\};", IMAGE_TABLES_CPP.read_text(), re.S + r"kImageTableKeys\[IMAGE_TABLE_COUNT\] = \{(.*?)\};", IMAGE_TABLES_CPP.read_text(), re.DOTALL ) assert body, "kImageTableKeys not found — has the parser been restructured?" return re.findall(r'"([a-z_]+)"', body.group(1)) @@ -79,7 +82,7 @@ def _struct_fields() -> list[str]: added, removed or reordered, not to have an opinion on how it is spelled. """ body = re.search( - r"typedef struct\s*\{(.*?)\}\s*image_tables_t", IMAGE_TABLES_H.read_text(), re.S + r"typedef struct\s*\{(.*?)\}\s*image_tables_t", IMAGE_TABLES_H.read_text(), re.DOTALL ) assert body, "image_tables_t not found — has the header been restructured?" lines = [line for line in body.group(1).splitlines() if line.strip().startswith(("IEC_",))] @@ -97,9 +100,30 @@ def _c_units() -> list[str]: return re.findall(r'"([a-z]+)"', body.group(1)) +def _python_plugin_order() -> list[str]: + """`IMAGE_TABLE_ORDER` in the shared Python module plugins import. + + The FOURTH copy of the order, and the one nothing pinned. It is what turns + the runtime's positional `sizes` array into the names every Python buffer + accessor uses, so a reorder of `image_table_id_t` would leave CI green + while every Python plugin silently read one table's length as another's. + Read as text rather than imported, like the C readers, because importing + the plugin package needs its virtualenv. + """ + src = (REPO_ROOT / "core/src/drivers/plugins/python/shared/image_sizes.py").read_text() + body = re.search(r"IMAGE_TABLE_ORDER: list\[str\] = \[(.*?)\]", src, re.DOTALL) + assert body, "IMAGE_TABLE_ORDER not found — has the module been restructured?" + return re.findall(r'"([a-z_]+)"', body.group(1)) + + @pytest.mark.parametrize( "name,reader", - [("enum", _enum_ids), ("key array", _c_keys), ("struct", _struct_fields)], + [ + ("enum", _enum_ids), + ("key array", _c_keys), + ("struct", _struct_fields), + ("python plugin order", _python_plugin_order), + ], ) def test_the_c_side_lists_agree_with_python_exactly(name, reader): # Order matters as much as membership: the key array is indexed BY the enum, @@ -228,3 +252,72 @@ def test_the_two_enums_really_do_disagree_on_order(self): def test_the_journal_covers_every_table_the_image_has(self): assert sorted(self._journal_ids()) == sorted(image_config.IMAGE_TABLE_KEYS) + + +class TestTheOtherTableMappings: + """Three more name-to-name maps over the same fourteen tables. + + `kJournalToImageTable` is pinned by TestJournalMapping. These three are the + same shape and were pinned by nothing, which matters because Ceedling does + not run in CI -- nothing compile-checks the two C ones either. A single + wrong entry writes or reads under another table's bounds with no + diagnostic, which is the failure the whole enum-order finding was about. + """ + + @staticmethod + def _pairs(path, pattern) -> dict[str, str]: + return {a.lower(): b.lower() for a, b in re.findall(pattern, path.read_text())} + + def test_s7comm_maps_every_buffer_type_to_the_table_of_the_same_name(self): + pairs = self._pairs( + S7COMM_C, r"case BUFFER_TYPE_([A-Z_]+):\s*return IMAGE_TABLE_([A-Z_]+);" + ) + assert pairs, "s7_image_table not found — has the plugin been restructured?" + assert sorted(pairs) == sorted(image_config.IMAGE_TABLE_KEYS) + for buffer_type, table in pairs.items(): + assert ( + buffer_type == table + ), f"BUFFER_TYPE_{buffer_type.upper()} maps to the wrong table" + + def test_ethercat_maps_each_direction_and_width_to_the_right_pair(self): + src = ETHERCAT_C.read_text() + body = re.search(r"ecat_table_for\(.*?\n\}", src, re.DOTALL) + assert body, "ecat_table_for not found — has the plugin been restructured?" + + # EtherCAT only ever emits %I and %Q, so there is no memory case. + expected = { + "BIT": ("BOOL_INPUT", "BOOL_OUTPUT"), + "BYTE": ("BYTE_INPUT", "BYTE_OUTPUT"), + "WORD": ("INT_INPUT", "INT_OUTPUT"), + "DWORD": ("DINT_INPUT", "DINT_OUTPUT"), + "LWORD": ("LINT_INPUT", "LINT_OUTPUT"), + } + for size, (in_table, out_table) in expected.items(): + # The case label and its return sit on separate lines after + # clang-format, so the match has to span them. + arm = re.search( + rf"case IEC_SIZE_{size}:\s*return in \? IMAGE_TABLE_(\w+) : IMAGE_TABLE_(\w+);", + body.group(0), + ) + assert arm, f"IEC_SIZE_{size} is not mapped" + assert arm.group(1) == in_table, f"IEC_SIZE_{size} input side is wrong" + assert arm.group(2) == out_table, f"IEC_SIZE_{size} output side is wrong" + + def test_the_modbus_segments_name_the_tables_they_live_in(self): + body = re.search(r"SEGMENT_TABLES = \{(.*?)\}", MODBUS_PY.read_text(), re.DOTALL) + assert body, "SEGMENT_TABLES not found" + segments = dict(re.findall(r'"(\w+)":\s*"(\w+)"', body.group(1))) + + # All eight, not the three the behavioural tests happen to exercise. + assert segments == { + "qw_count": "int_output", + "mw_count": "int_memory", + "md_count": "dint_memory", + "ml_count": "lint_memory", + "qx_bits": "bool_output", + "mx_bits": "bool_memory", + "ix_bits": "bool_input", + "iw_count": "int_input", + } + for table in segments.values(): + assert table in image_config.IMAGE_TABLE_KEYS diff --git a/tests/pytest/test_plugins_declare_image_sizes.py b/tests/pytest/test_plugins_declare_image_sizes.py new file mode 100644 index 00000000..a1c41ac0 --- /dev/null +++ b/tests/pytest/test_plugins_declare_image_sizes.py @@ -0,0 +1,81 @@ +"""Every plugin the runtime ships has to declare per-table image sizes. + +The runtime keeps the image SQUARE for any run in which even one loaded plugin +lacks `set_image_sizes` -- correctly, because a plugin bounding a byte index +and a word index with one `buffer_size` is only right while the tables are +equal. The consequence is that ONE plugin without it disables the feature for +the whole device. + +That is exactly what shipped: `simple_modbus.py` had it and +`modbus_master_plugin.py` and `opcua/plugin.py` did not, so per-table sizing +never activated on a stock `plugins_default.conf` and every fix beneath it ran +in the degenerate case where it could not differ from the old behaviour. + +Nothing failed. The image was simply square, which is also what a correct +square run looks like. This test is the only thing that tells the two apart. +""" + +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +PLUGINS_CONF = REPO_ROOT / "plugins_default.conf" +PYTHON_PLUGINS = REPO_ROOT / "core/src/drivers/plugins/python" + + +def shipped_entries() -> list[tuple[str, str]]: + """`(name, path)` for every plugin the default config loads.""" + out = [] + for raw in PLUGINS_CONF.read_text().splitlines(): + line = raw.strip() + if not line or line.startswith("#"): + continue + fields = line.split(",") + if len(fields) >= 2: + out.append((fields[0].strip(), fields[1].strip())) + return out + + +def test_the_default_config_is_readable(): + # A guard on the guard: a rename or a move that makes the parse return + # nothing would otherwise turn this whole suite into a silent pass. + assert len(shipped_entries()) >= 5 + + +@pytest.mark.parametrize( + "name,path", + [(n, p) for n, p in shipped_entries() if p.endswith(".py")], +) +def test_every_shipped_python_plugin_declares_set_image_sizes(name, path): + source = (REPO_ROOT / path.lstrip("./")).read_text() + # The NAME has to be in this module's namespace, which is where + # PyObject_GetAttrString looks. Importing it from shared.image_sizes is how + # that is done; defining it directly would also work. + assert re.search( + r"^\s*(from .*import .*\bset_image_sizes\b|def set_image_sizes\b)", source, re.MULTILINE + ), ( + f"plugin '{name}' ({path}) does not declare set_image_sizes, so every run " + f"it is loaded in keeps the image square" + ) + + +def test_every_shipped_native_plugin_links_the_helper(): + # The native side declares it by linking plugin_image_sizes.c, which + # defines and exports the symbol. Checked through the build files, because + # the .so is not in the tree. + native = [p for _, p in shipped_entries() if p.endswith(".so")] + assert native, "no native plugin in the default config — has it been restructured?" + + for cmake in (REPO_ROOT / "core/src/drivers/plugins/native").glob("*/CMakeLists.txt"): + assert "plugin_image_sizes.c" in cmake.read_text(), ( + f"{cmake.parent.name} does not link plugin_image_sizes.c, so it exports no " + f"set_image_sizes and every run it is loaded in keeps the image square" + ) + + +def test_the_shared_module_is_what_they_import(): + # One implementation rather than one per plugin: three copies of a + # fourteen-element cache is three places for the indexing to drift. + assert (PYTHON_PLUGINS / "shared/image_sizes.py").exists() From da21ef6902deb56f82171997ab1e3a94dc68d3e6 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 17:17:43 -0300 Subject: [PATCH 4/5] fix(image): the nits, and the bound now comes from where the pointers 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 --- core/src/drivers/plugin_driver.c | 31 ++++++++++--- core/src/drivers/plugin_types.h | 15 ++++-- .../plugins/native/s7comm/s7comm_plugin.cpp | 9 ++-- .../plugins/python/shared/image_sizes.py | 15 +++++- core/src/plc_app/image_tables.cpp | 5 +- core/src/plc_app/journal_buffer.c | 9 +++- core/src/plc_app/journal_buffer.h | 35 ++++++++++++-- core/src/plc_app/plc_state_manager.cpp | 46 ++++++++++++------- tests/pytest/test_modbus_exposure_fit.py | 20 ++++++++ 9 files changed, 150 insertions(+), 35 deletions(-) diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index ae8aa5d5..29e98ddd 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -1257,12 +1257,19 @@ void *generate_structured_args_with_driver(plugin_type_t type, plugin_driver_t * sizeof(driver->plugins[plugin_index].config.plugin_related_config_path)); // Initialize buffer size info - /* The allocated size, not a compile-time constant. Plugins bounds-check - * against this field -- ethercat_io.c refuses a byte_index at or above it, - * s7comm derives every clamp from it -- so it has to describe the image - * that actually exists. It describes all fourteen tables because they are - * all allocated at the same count; see image_sizes_flatten() for why the - * ABI leaves no room for anything else. */ + /* THE SMALLEST OF THE FOURTEEN, not the length they all share. + * + * The tables no longer have one length, and this field cannot say so -- + * CON06 keeps the struct's offsets fixed. The minimum is the only safe + * single number for a consumer that has not been told they can differ: + * bounding by it refuses an index, where bounding by the largest reads + * past every shorter table. + * + * ethercat_io.c and s7comm no longer derive their clamps from this field; + * they export set_image_sizes and bound by the table each access actually + * addresses, falling back here only when the sizes were never delivered. + * plugin_types.h carries the same statement for plugin authors, and + * journal_buffer.h for the runtime's own copy. */ args->buffer_size = (int)image_tables_capacity(); args->bits_per_buffer = 8; @@ -1472,6 +1479,10 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // start_loop is optional Py_XDECREF(py_binds->pFuncStart); py_binds->pFuncStart = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); } py_binds->pFuncStop = PyObject_GetAttrString(py_binds->pModule, "stop_loop"); @@ -1480,6 +1491,10 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // stop_loop is optional Py_XDECREF(py_binds->pFuncStop); py_binds->pFuncStop = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); } py_binds->pFuncSetImageSizes = PyObject_GetAttrString(py_binds->pModule, "set_image_sizes"); @@ -1501,6 +1516,10 @@ int python_plugin_get_symbols(plugin_instance_t *plugin) // cleanup is optional Py_XDECREF(py_binds->pFuncCleanup); py_binds->pFuncCleanup = NULL; + /* A failed PyObject_GetAttrString leaves an AttributeError SET, and an + * optional lookup does not return, so it has to be cleared here or the + * next CPython call reports this absence as its own failure. */ + PyErr_Clear(); } // Store the python binds in the plugin instance diff --git a/core/src/drivers/plugin_types.h b/core/src/drivers/plugin_types.h index a7b34dee..d4c25f04 100644 --- a/core/src/drivers/plugin_types.h +++ b/core/src/drivers/plugin_types.h @@ -257,9 +257,18 @@ typedef struct * kept square for that run and this field is again the length they all * have. * - * Not marked deprecated yet, deliberately: the build carries -Werror, so - * the attribute would fail the build for every consumer still reading it - * rather than naming them. It goes in once they are migrated. */ + * Not marked deprecated yet, and the reason is not what an earlier draft + * of this comment claimed. The runtime core does build with -Werror + * (core/src/CMakeLists.txt), but the plugins do not: they are configured + * by their own cmake invocation and the VPP packages by a plain Makefile, + * so the attribute would produce warnings there, not a build failure. + * + * It is deferred because 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 set_image_sizes can use. Deprecating it now + * would warn at correct code, including in packages that ship + * independently and must keep working against older runtimes. The + * attribute goes in once the symbol is universal. */ int buffer_size; int bits_per_buffer; diff --git a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp index 7525a815..1753b507 100644 --- a/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp +++ b/core/src/drivers/plugins/native/s7comm/s7comm_plugin.cpp @@ -756,9 +756,12 @@ static int get_type_size(s7comm_buffer_type_t type) * * A THIRD order for the same fourteen tables. This enum groups each width's * memory beside its input and output, matching journal_buffer_type_t; - * image_table_id_t puts every memory table at the end. BUFFER_TYPE_INT_MEMORY - * is 7 and IMAGE_TABLE_INT_MEMORY is 10, so a cast between them reads and - * writes under another table's bounds. Written out rather than computed. */ + * image_table_id_t puts every memory table at the end -- and this enum starts + * at BUFFER_TYPE_NONE, so it is offset again. For one table, int_memory: + * BUFFER_TYPE_INT_MEMORY is 8, JOURNAL_INT_MEMORY is 7, IMAGE_TABLE_INT_MEMORY + * is 10. Three different numbers for one table is the whole argument, and a + * cast between any two reads and writes under another table's bounds. Written + * out rather than computed. */ static image_table_id_t s7_image_table(s7comm_buffer_type_t type) { switch (type) diff --git a/core/src/drivers/plugins/python/shared/image_sizes.py b/core/src/drivers/plugins/python/shared/image_sizes.py index 56e8526a..4885ab21 100644 --- a/core/src/drivers/plugins/python/shared/image_sizes.py +++ b/core/src/drivers/plugins/python/shared/image_sizes.py @@ -58,17 +58,28 @@ def set_image_sizes(sizes) -> int: Returns 0 on success, which is what the runtime requires; non-zero fails the plugin exactly as a failed ``init`` does. """ - _sizes.clear() + # Built into a local and published only on success. _sizes is + # PROCESS-GLOBAL -- this module is imported once per interpreter and every + # Python plugin in that process shares it -- so a half-filled map left + # behind by a plugin being torn down would answer for the tables before + # the failure and fall back to buffer_size for the rest, in plugins that + # are still running. + parsed: dict[str, int] = {} try: values = list(sizes) except TypeError: + _sizes.clear() return -1 for name, count in zip(IMAGE_TABLE_ORDER, values): try: - _sizes[name] = int(count) + parsed[name] = int(count) except (TypeError, ValueError): + _sizes.clear() return -1 + + _sizes.clear() + _sizes.update(parsed) return 0 diff --git a/core/src/plc_app/image_tables.cpp b/core/src/plc_app/image_tables.cpp index a934f0f8..076b718f 100644 --- a/core/src/plc_app/image_tables.cpp +++ b/core/src/plc_app/image_tables.cpp @@ -1189,6 +1189,10 @@ extern "C" bool image_tables_alloc(const image_sizes_t *sizes) t_int_memory = (IEC_UINT *)calloc(N(IMAGE_TABLE_INT_MEMORY), sizeof(IEC_UINT)); t_dint_memory = (IEC_UDINT *)calloc(N(IMAGE_TABLE_DINT_MEMORY), sizeof(IEC_UDINT)); t_lint_memory = (IEC_ULINT *)calloc(N(IMAGE_TABLE_LINT_MEMORY), sizeof(IEC_ULINT)); +/* Undefined right after the last use, not inside a runtime branch: the + * preprocessor does not care which branch it sits in, so putting it in the + * failure path only worked because every use happened to be above it. */ +#undef N const bool complete = next.bool_input && next.bool_output && next.bool_memory && next.byte_input && next.byte_output && next.int_input && @@ -1230,7 +1234,6 @@ extern "C" bool image_tables_alloc(const image_sizes_t *sizes) free(t_dint_memory); free(t_lint_memory); log_error("[image_tables] could not allocate the image; the previous one is untouched"); -#undef N return false; } diff --git a/core/src/plc_app/journal_buffer.c b/core/src/plc_app/journal_buffer.c index 7f0e3600..8093956f 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -138,6 +138,13 @@ static const image_table_id_t kJournalToImageTable[JOURNAL_TYPE_COUNT] = { [JOURNAL_LINT_MEMORY] = IMAGE_TABLE_LINT_MEMORY, }; +image_table_id_t journal_type_to_image_table(uint8_t type) +{ + if (type >= JOURNAL_TYPE_COUNT) + return IMAGE_TABLE_COUNT; + return kJournalToImageTable[type]; +} + /** The longest table, which is how long a forced-slot row has to be: rows are * one length for all fourteen types, so the longest is the only one that can * record a forced slot anywhere any table reaches. Under-allocating here is @@ -147,7 +154,7 @@ static uint32_t journal_longest_table(void) uint32_t longest = 0; for (int t = 0; t < JOURNAL_TYPE_COUNT; ++t) { - const uint32_t n = image_table_capacity(kJournalToImageTable[t]); + const uint32_t n = g_buffer_ptrs.table_sizes[t]; if (n > longest) longest = n; } diff --git a/core/src/plc_app/journal_buffer.h b/core/src/plc_app/journal_buffer.h index 39992c54..e3d55cc5 100644 --- a/core/src/plc_app/journal_buffer.h +++ b/core/src/plc_app/journal_buffer.h @@ -25,11 +25,12 @@ #ifndef JOURNAL_BUFFER_H #define JOURNAL_BUFFER_H +#include "../lib/iec_types.h" +#include "image_table_id.h" +#include #include -#include #include -#include -#include "../lib/iec_types.h" +#include #ifdef __cplusplus extern "C" { @@ -115,6 +116,19 @@ typedef struct { IEC_ULINT **lint_output; IEC_ULINT **lint_memory; + /* How long each array above is, in its own elements, indexed by + * journal_buffer_type_t. + * + * Taken at journal_init, from the SAME moment as the pointers beside it. + * The bound and the pointers have to come from one point in time: reading + * the live image sizes while holding pointers captured earlier would, if + * the two ever diverged, apply a new length to an old allocation. 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. */ + uint32_t table_sizes[JOURNAL_TYPE_COUNT]; + /* THE SMALLEST ARRAY, NOT THE LENGTH OF ALL OF THEM (RTOP-284). * * The arrays above no longer share a length. This field was a second copy @@ -145,6 +159,21 @@ typedef struct { * * @return Drops since the last call. */ +/** + * @brief Which image table a journal buffer type stores. + * + * The two enums name the same fourteen tables in DIFFERENT orders -- the + * journal puts each width's memory beside its input and output, image_tables.h + * groups the memory tables at the end -- so a cast between them lands under + * another table's bounds. Exposed so the caller filling `table_sizes` uses the + * same mapping the journal itself does rather than a second copy of it. + * + * @param type A `journal_buffer_type_t`. + * @return The matching `image_table_id_t`, or `IMAGE_TABLE_COUNT` if the type + * is out of range. + */ +image_table_id_t journal_type_to_image_table(uint8_t type); + unsigned journal_take_force_drops(void); /** diff --git a/core/src/plc_app/plc_state_manager.cpp b/core/src/plc_app/plc_state_manager.cpp index c5e705e6..6d3a6212 100644 --- a/core/src/plc_app/plc_state_manager.cpp +++ b/core/src/plc_app/plc_state_manager.cpp @@ -453,26 +453,40 @@ void *plc_cycle_thread(void *arg) plc_retain_read(); journal_buffer_ptrs_t journal_ptrs = { - .bool_input = g_image.bool_input, - .bool_output = g_image.bool_output, - .bool_memory = g_image.bool_memory, - .byte_input = g_image.byte_input, - .byte_output = g_image.byte_output, - .int_input = g_image.int_input, - .int_output = g_image.int_output, - .int_memory = g_image.int_memory, - .dint_input = g_image.dint_input, - .dint_output = g_image.dint_output, - .dint_memory = g_image.dint_memory, - .lint_input = g_image.lint_input, - .lint_output = g_image.lint_output, - .lint_memory = g_image.lint_memory, + .bool_input = g_image.bool_input, + .bool_output = g_image.bool_output, + .bool_memory = g_image.bool_memory, + .byte_input = g_image.byte_input, + .byte_output = g_image.byte_output, + .int_input = g_image.int_input, + .int_output = g_image.int_output, + .int_memory = g_image.int_memory, + .dint_input = g_image.dint_input, + .dint_output = g_image.dint_output, + .dint_memory = g_image.dint_memory, + .lint_input = g_image.lint_input, + .lint_output = g_image.lint_output, + .lint_memory = g_image.lint_memory, /* Follows the image: journal_buffer.c bounds every forced write * against this, so a stale constant here would silently drop writes to * the part of the image beyond it. */ - .buffer_size = (int)image_tables_capacity(), - .image_mutex = itm, + .table_sizes = {}, + .buffer_size = (int)image_tables_capacity(), + .image_mutex = itm, }; + + /* The fourteen lengths, captured HERE, at the same moment as the pointers + * above. journal_buffer.c bounds each write by the table it addresses, and + * reading that from the live image while holding pointers taken earlier + * would apply a new length to an old allocation if the two ever diverged. + * + * The journal's own order, not the image's -- they are the same fourteen + * tables in different orders, which is the trap kJournalToImageTable + * exists for. */ + for (int t = 0; t < JOURNAL_TYPE_COUNT; ++t) + { + journal_ptrs.table_sizes[t] = image_table_capacity(journal_type_to_image_table(t)); + } if (journal_init(&journal_ptrs) != 0) { /* FATAL, not a log line, and this is newly true. diff --git a/tests/pytest/test_modbus_exposure_fit.py b/tests/pytest/test_modbus_exposure_fit.py index 59385444..7ec5a0c5 100644 --- a/tests/pytest/test_modbus_exposure_fit.py +++ b/tests/pytest/test_modbus_exposure_fit.py @@ -289,3 +289,23 @@ def test_the_module_exports_the_symbol_the_runtime_looks_for(sm): # in this module's namespace -- importing it is what declares the # capability, and without it every run this plugin is in stays square. assert callable(getattr(sm, "set_image_sizes", None)) + + +def test_a_failed_delivery_leaves_no_half_filled_map(sm): + """A mid-list failure must not answer for the tables before it. + + `_sizes` is process-global: the module is imported once per interpreter and + every Python plugin in that process shares it. A plugin being torn down on + a bad delivery could otherwise leave a partial map behind for plugins that + keep running -- answering for the tables it parsed and falling back to + buffer_size for the rest. + """ + from shared import image_sizes + + assert image_sizes.set_image_sizes([10] * 14) == 0 + assert image_sizes.sizes_known() + + # Fails on the third entry, after two were parsed. + assert image_sizes.set_image_sizes([1, 2, "nao-e-numero", 4]) == -1 + assert not image_sizes.sizes_known() + assert image_sizes.table_capacity("bool_input", 99) == 99 From cb89382bd3d7e12621c5295023f19a3691562510 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 18:32:34 -0300 Subject: [PATCH 5/5] fix(journal): take the bound from the same moment as the pointers 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 --- core/src/plc_app/journal_buffer.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/core/src/plc_app/journal_buffer.c b/core/src/plc_app/journal_buffer.c index e5830148..b376f9c9 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -161,12 +161,30 @@ static uint32_t journal_longest_table(void) return longest; } -/** How far this journal type's table actually reaches. */ +/** + * How far this journal type's table actually reaches. + * + * FROM THE SNAPSHOT, not from the live image sizes, and the two are not the + * same question. `g_buffer_ptrs.table_sizes[]` was filled at journal_init from + * the SAME moment as the table pointers beside it; `image_table_capacity()` + * reads `g_sizes`, which `image_tables_alloc` rewrites under the image-tables + * mutex that this path does not hold. Every caller here uses the result to + * index one of those 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. + * + * The two cannot diverge today -- the image is allocated before the cycle + * thread that calls journal_init exists, and a re-load stops that thread + * first -- so this is not a bug being fixed. It is the implicit coupling made + * explicit, which is what the rest of this task has been doing, and it also + * turns a non-inlinable cross-TU call per journal entry on the drain path back + * into the struct field read it used to be. + */ static uint32_t journal_type_capacity(uint8_t type) { if (type >= JOURNAL_TYPE_COUNT) return 0; - return image_table_capacity(kJournalToImageTable[type]); + return g_buffer_ptrs.table_sizes[type]; } static uint8_t *g_forced[JOURNAL_TYPE_COUNT];