diff --git a/core/src/drivers/plugin_driver.c b/core/src/drivers/plugin_driver.c index 66ed2d5a..29e98ddd 100644 --- a/core/src/drivers/plugin_driver.c +++ b/core/src/drivers/plugin_driver.c @@ -591,6 +591,122 @@ 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; + + /* 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) + 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 +748,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 +799,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) @@ -1120,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_largest() 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; @@ -1335,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"); @@ -1343,6 +1491,23 @@ 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"); + 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"); @@ -1351,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 @@ -1487,6 +1656,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 8ba93637..4076a71f 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 @@ -203,6 +229,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..d4c25f04 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 @@ -229,7 +239,36 @@ 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, 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/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..1753b507 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,72 @@ 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 -- 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) + { + 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 +843,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 +882,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 +916,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 +950,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 +1012,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 +1035,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 +1054,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 +1073,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_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/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/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/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..4885ab21 --- /dev/null +++ b/core/src/drivers/plugins/python/shared/image_sizes.py @@ -0,0 +1,98 @@ +"""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. + """ + # 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: + parsed[name] = int(count) + except (TypeError, ValueError): + _sizes.clear() + return -1 + + _sizes.clear() + _sizes.update(parsed) + 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..076b718f 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,41 @@ 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)); +/* 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 && @@ -1185,9 +1233,7 @@ 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"); return false; } @@ -1209,36 +1255,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 +1367,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 463cf059..b376f9c9 100644 --- a/core/src/plc_app/journal_buffer.c +++ b/core/src/plc_app/journal_buffer.c @@ -50,6 +50,7 @@ unsigned journal_take_force_drops(void) */ #include "journal_buffer.h" +#include "image_tables.h" #include "utils/log.h" #include "utils/utils.h" #include @@ -109,6 +110,83 @@ 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, +}; + +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 + * 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 = g_buffer_ptrs.table_sizes[t]; + if (n > longest) + longest = n; + } + return longest; +} + +/** + * 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 g_buffer_ptrs.table_sizes[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 @@ -186,7 +264,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)) { @@ -203,13 +288,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; } @@ -383,7 +472,21 @@ static void apply_entry(const journal_entry_t *entry) * under image_lock — the same serialization domain as apply_entry. */ int 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. */ @@ -414,7 +517,21 @@ int journal_force_set(journal_buffer_type_t type, uint16_t index, uint8_t bit, u * plugin) is no longer dropped, so the slot tracks the live value again. */ int 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. */ @@ -494,13 +611,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; } @@ -685,10 +802,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 4e7e64c8..e240b6aa 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,7 +116,29 @@ typedef struct { IEC_ULINT **lint_output; IEC_ULINT **lint_memory; - /* Buffer size (number of elements in each array) */ + /* 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 + * 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) */ @@ -136,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_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 f2730350..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. @@ -1095,9 +1109,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) @@ -1111,8 +1146,7 @@ extern "C" int load_plc_program(PluginManager *pm) plc_state = PLC_STATE_ERROR; pthread_mutex_unlock(&state_mutex); log_info("PLC State: ERROR"); - if (pm == plc_program) - plc_program = NULL; + if (pm == plc_program) plc_program = NULL; plugin_manager_destroy(pm); return -1; } diff --git a/tests/pytest/test_image_conf_contract.py b/tests/pytest/test_image_conf_contract.py index 88b93fad..a3239013 100644 --- a/tests/pytest/test_image_conf_contract.py +++ b/tests/pytest/test_image_conf_contract.py @@ -40,22 +40,33 @@ # 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" +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_TABLES_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 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"] 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)) @@ -71,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_",))] @@ -89,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, @@ -160,3 +192,132 @@ 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) + + +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_modbus_exposure_fit.py b/tests/pytest/test_modbus_exposure_fit.py index 532beeb4..7ec5a0c5 100644 --- a/tests/pytest/test_modbus_exposure_fit.py +++ b/tests/pytest/test_modbus_exposure_fit.py @@ -232,3 +232,80 @@ 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)) + + +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 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()