Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Built-in modules without class maps incorrectly inherit the global built-in class maps array, violating state invariants and causing performance regressions on queries. --
commit 97c08d5dc7f7ec501676f73addba8ad9f6c67352 Author: Jim Cromie <[email protected]> dyndbg,module: make proper substructs in _ddebug_info This patch recomposes struct _ddebug_info by inserting proper sub-structs for descriptors and class maps. It updates the usage of this struct as a cursor for built-in module data and module state access. Note that the regression detailed below was fixed later in the patch series by commit b54eb6f1ef1ecdd4f503388db9a2426bb84d49d1. > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c > --- a/lib/dynamic_debug.c > +++ b/lib/dynamic_debug.c > @@ -1176,12 +1177,12 @@ static void ddebug_attach_module_classes(struct > ddebug_table *dt, struct _ddebug > * the builtin/modular classmap vector/section. Save the start > * and length of the subrange at its edges. > */ > - for_subvec(i, cm, di, classes) { > + for_subvec(i, cm, di, maps) { > if (!strcmp(cm->mod_name, dt->mod_name)) { [ ... ] > } else if (nc) { > /* end of matching classmaps */ > break; > } > } > if (nc) { > - dt->num_classes = nc; > + dt->info.maps.len = nc; [Severity: Medium] When a built-in module does not have any class maps (so nc remains 0), this function will exit without setting dt->info.maps.len to 0. Because dt->info was populated by a shallow copy of *di earlier in ddebug_add_module(), this leaves dt->info.maps.len set to the global built-in class maps array length. Does this cause modules without class maps to incorrectly retain the entire global class maps array bounds? This appears to degrade query time complexity, as ddebug_change() will falsely find class maps in these modules and needlessly iterate over all their descriptors. > vpr_info("module:%s attached %d classes\n", dt->mod_name, nc); > } > } [ ... ] > @@ -1220,19 +1221,18 @@ static int ddebug_add_module(struct _ddebug_info *di, > const char *modname) > * this struct ddebug_table. > */ > dt->mod_name = modname; > - dt->ddebugs = di->descs; > - dt->num_ddebugs = di->num_descs; > + dt->info = *di; [Severity: Medium] This shallow struct copy initializes the module's maps with the global array bounds. Before this patch, dt->num_classes was correctly initialized to 0 by kzalloc_obj(*dt) and only updated if classes were found. If ddebug_attach_module_classes() doesn't overwrite dt->info.maps.len for modules with no class maps, does this break the contiguous subrange invariant? > > INIT_LIST_HEAD(&dt->link); > > - if (di->classes && di->num_classes) > + if (di->maps.len) > ddebug_attach_module_classes(dt, di); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=19
