Fix GUI freezes on large netlists, a nullptr crash in Net, and complete the plugin manager bindings

[julianspeith]

A collection of unrelated fixes, kept in one branch as they are all self contained. The first group came out of profiling the GUI on the OpenTitan benchmark of the HAWKEYE artifacts, 424866 gates in a single module, the rest out of going through the documentation.

The GUI froze or stalled on large netlists

Selecting a module hung the GUI indefinitely. ModuleModel::populateTree() correctly brackets the population with beginResetModel() and endResetModel(), but createChildItem() emitted beginInsertRows() and endInsertRows() for every single item in between. Besides being undefined per the QAbstractItemModel contract, it made the attached QSortFilterProxyModel remap its source rows once per inserted row, which is quadratic in the number of items.

A sample taken while the GUI was busy put 10543 of 16692 main thread samples below QAbstractItemModel::endInsertRows(), and it was still in the same place at 100% CPU nine minutes later. Measured separately with a QSortFilterProxyModel over a plain list model, populating with a row signal per item takes 0.84 s for 50000 items, 3.29 s for 100000 and 14.71 s for 200000, while populating within a plain model reset is too fast to measure. OpenTitan's top module holds all of its 424866 gates directly, so this was about 1.8e11 units of work instead of 4.2e5.

The same two functions also reset mIsModifying to false unconditionally, clobbering the true that populateTree() had set, so the guard in ModuleWidget was only in effect for the very first item. mIsModifying was never initialized in the constructor either.

Unfolding a large module took seconds. Without uniform row heights, QTreeViewPrivate::itemHeight() measures every row individually through QTreeView::indexRowSizeHint(), which asks the item delegate for a size hint, which lays out and shapes the item text. A sample taken while unfolding shows 5094 of 8032 main thread samples below itemHeight(), of which 2345 end in QTextEngine::shapeText() and 821 in CTFontGetGlyphsForCharacters(): the view spent the unfold shaping gate names just to learn how tall their rows are. All rows of the four affected views are plain single line text in one font.

The module elements tree was rebuilt twice per selection. SelectionDetailsWidget::handleSelectionUpdate() drives the details widget directly and also populates the selection tree, whose current index change drives the very same widget again through triggerSelection(). A sample shows populateTree() running three times for one selection, 321, 313 and 307 samples of 1358 in that branch, each building one item per gate and per net of the module.

Crash on a nullptr pin, reachable from Python

Net::remove_source(Gate*, const GatePin*) and Net::remove_destination(Gate*, const GatePin*) dereferenced the pin without checking it:

std::find_if(..., [gate, pin](auto ep) { return ep->get_gate() == gate && *ep->get_pin() == *pin; });

The string overloads never reach that state, because they reject an unknown pin name before delegating. The pin overloads, however, are called directly by netlist_preprocessing and netlist_utils, and they are bound to Python, where this was enough to terminate the interpreter:

net.is_a_source(gate, None)      # -> False
net.remove_source(gate, None)    # -> process dies

is_a_source and is_a_destination already guarded both arguments; the two removal overloads now do the same, so the call returns False and logs a warning.

Pin lookup compared pins by value instead of by identity

The same lambdas compared *ep->get_pin() == *pin, so a distinct but equal GatePin of another gate type would match. Every pin of a gate comes from that gate's type, so identity is what these lookups mean. Changed in the four Net lookups and in Gate::get_fan_in_endpoint / get_fan_out_endpoint.

Net::operator== keeps comparing by value on purpose: it compares endpoints of two different nets, which do not share pin objects.

Missing Python bindings of the plugin manager

plugin_manager.h carried seven TODO Python binding markers. All are now resolved:

• get_cli_plugin_flags() and get_ui_plugin_flags() return a dict.
• add_model_changed_callback() and remove_model_changed_callback() let a Python callable observe plugins being loaded and unloaded.
• get_plugin_instance() gained the initialize and silent parameters. The other half of that TODO, "bindings for different types", needed no work: pybind11 already returns the plugin's own type once that plugin's Python module has been imported, and the base interface otherwise. The docstring now says so.
• add_existing_options_description(), get_cli_plugin_options(), get_plugin_path(), has_valid_file_extension() and get_plugin_features() required binding the types they use, so ProgramOptions, ProgramArguments, FacExtensionInterface with its Feature enum, and plugin_manager::PluginFeature are now bound as well.

Two details worth a look during review:

ProgramOptions::add_flags() is new. add() takes its flags as a std::initializer_list, which cannot be built from a container at runtime and is therefore not bindable. Retyping the parameter to std::vector is not an option either: add({"-h", "--help"}, "...") then becomes ambiguous, because std::string has an iterator-pair constructor and the braced list is a viable argument for the single-flag overload as well. The vector-based implementation is therefore exposed under the new name add_flags(), to which both existing overloads delegate. All 87 existing call sites are untouched, and Python binds add_flags() as add().

ProgramArguments::get_original_arguments() is deliberately not bound. ProgramArguments stores the argv pointer without owning the strings, which no longer exist once parse() has returned to Python.

Faster Boolean function comparison

BooleanFunction::operator< compared two functions of equal node count by building their reverse polish notation strings and comparing those. The symbolic state keeps its variables in a std::map<BooleanFunction, BooleanFunction>, so every variable lookup during evaluation formatted several strings, and since all keys are single variable nodes the node count never discriminated. Comparing the nodes directly is equivalent, Node::operator< is already a field wise comparison consistent with Node::operator==. Neither of the two maps keyed by a Boolean function is ever iterated, so the changed order is not observable.

Together with two smaller fixes in the same path, constant_propagation() copying the value vector of every operand twice and evaluate() validating its inputs by comparing every input name against every node, this takes a truth table of a 50 node function over 8 variables from 5.17 ms to 3.81 ms. PR #636 builds on this.

HAWKEYE S-box database and the crypto_trojan example

The S-box database is now copied into the build directory so that it is found at runtime, the way the gate library definitions already are, and the crypto_trojan and toy_cipher examples were refreshed. identify_sbox() returning an empty string means that no S-box of the database matched, which is not an error; the documentation said otherwise.

Removed the stale Python binding tests

tests/python_binding/ is not referenced by the build or by any workflow, so nothing has run these files in a long time. They would not run either: they still call boolean_function(), value.ONE, get_variables(), get_truth_table(), is_constant_zero(), to_dnf() and optimize(), none of which are bound any more. The C++ suite under tests/netlist covers the same ground.

Testing

• Added direct tests for the two pin overloads of remove_source and remove_destination, covering removal, the wrong pin, a nullptr pin, a nullptr gate, a gate that is not connected, and removing twice. They previously had no direct coverage at all, which is what the removed TODO test comments asked for. The other six of those TODOs were stale: the string overloads, the endpoint overloads, and both pin overloads of is_a_source and is_a_destination are already covered.
• Verified from Python that the crash is gone, that the plugin callback fires once per plugin and stops after removal, that both flag getters return a dict, that get_plugin_instance(name, False) skips initialization and get_plugin_instance(name, True, True) returns None for an unknown plugin without logging, and that options can be added with one or several flags, parsed, queried, removed and merged.
• runTest-net and runTest-gate pass in full, and the wider suite is unchanged.
• The GUI changes are verified by measurement rather than by a test, since the item models have no test coverage. The quadratic behaviour was reproduced and the fix confirmed with a standalone QSortFilterProxyModel benchmark, and the row height and duplicate rebuild costs were each identified in a sample of the running GUI. Worth exercising by hand on a large netlist: select the top module, unfold it, then select a module, a gate and the same module again, to confirm the details still populate correctly.

🤖 Generated with Claude Code

[joern274]

Explicit OK for GUI modifications