odb: guard instance module reassignment#10921
Conversation
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
There was a problem hiding this comment.
Code Review
This pull request refactors how instance module membership is managed by encapsulating the _dbInst::module_ field with getModuleId() and setModule(), ensuring that the inDbPostInstParentChange callback is correctly triggered during reparenting. It also updates _dbModule::removeInst to clean up the instance from the module's dbinst_hash_ and adds corresponding unit tests. The review feedback identifies a critical correctness issue where renaming an instance via dbInst::rename does not update the parent module's dbinst_hash_, which can leave stale entries and lead to undefined behavior or use-after-free upon destruction. A code suggestion is provided to update dbInst::rename to handle this case.
|
I also ran the focused TestModule suite under ASan with #10911 layered underneath for the hermetic sanitizer runtime. It passes with leak detection disabled. |
Signed-off-by: Naveen Venkat <archgen.guest@nyayanidhi.in>
| if (previous == 0 || module == 0) { | ||
| return; | ||
| } |
Fixes #10282.
_dbInst::module_could be assigned directly, so a future parent change could skipinDbPostInstParentChangeand leave hierarchical path caches stale. This makes the field private and routes normal changes throughsetModule(). Database serialization and hash rebuilding retain narrow friend access.While adding the regression I found that reparenting also left the instance in the old module name index.
removeInst()now removes that stale entry before linking the instance into its new module.The new test checks callback count and timing, same-parent no-op behavior, and both old and new module lookups.
Tested on the GCP VM:
bazel test //src/odb/test/cpp:all //src/dbSta/test:sdc_modinst_slash-tcl_testbazel build --config=lint //src/odb/src/db:db //src/odb/test/cpp:TestModuleAll 25 tests pass and the lint build passes.