gui: avoid call to sortItems since that triggers from bazel segfault - #11203
Conversation
Signed-off-by: Peter Gadfort <gadfort@zeroasic.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the sorting of help pages in HelpWidget::changeCategory() to avoid using QListWidget::sortItems(), which relies on loading ICU locale data. Instead, pages are collected into a vector, sorted manually, and then added to the list widget. Feedback suggests explicitly calling .string() on std::filesystem::path objects when converting them to QString to ensure cross-platform compatibility, as implicit conversion to std::string fails on Windows.
| pages.emplace_back(QString::fromStdString(path.stem()), | ||
| QString::fromStdString(path)); |
There was a problem hiding this comment.
Using path.stem() and path directly inside QString::fromStdString relies on the implicit conversion of std::filesystem::path to std::string. This implicit conversion only exists on POSIX platforms (where std::filesystem::path::value_type is char). On Windows, where the native path representation is wchar_t, this will fail to compile because the implicit conversion is to std::wstring instead.
To ensure cross-platform compatibility and avoid relying on platform-specific implicit conversions, explicitly call .string() on the path objects.
| pages.emplace_back(QString::fromStdString(path.stem()), | |
| QString::fromStdString(path)); | |
| pages.emplace_back(QString::fromStdString(path.stem().string()), | |
| QString::fromStdString(path.string())); |
Summary
Problem
A GUI-enabled
openroadthat has been installed dies with SIGSEGV the momentgui::showruns, on any design including none at all. This kills theimage-generation stage of every SiliconCompiler APR step, which drives the GUI
headlessly via
gui::show "source .../write_images.tcl" false.Reproducer, no design data needed:
Cause
HelpWidget::init()adds the help categories to aQComboBox. Qt auto-selectsrow 0 as soon as the first item lands, which fires
changeCategory(), whichcalled
help_list_->sortItems(). That sort compares throughQCollator, whoseconstructor loads ICU locale data:
The fault is in the Bazel Central Registry overlay for
icu, which patchesu_getDataDirectory()to find the packagedicudt76l.datthrough the runfilestree using three unchecked calls:
Runfiles::Create()is documented to returnnullptron error, and it failswhenever no runfiles tree is reachable; the next line dereferences it. Since
argv0is passed as"", rules_cc skips its<argv0>.runfilesfallback, soonly
$RUNFILES_DIRand$RUNFILES_MANIFEST_FILEare ever consulted --neither of which is set for an installed binary. Keeping a runfiles tree beside
the binary does not help for the same reason (verified).
Two conditions must hold, and
bazel/install.sharranges both: the man pagesare installed (line 64), so
HelpWidget::init()gets past its path validation;and
openroad.runfilesis deliberately removed after unpacking (lines 47-58).CLI builds are unaffected --
icureaches the build only viaqt-bazel, so//:openroad's runfiles contain noicu_dat.Fix
Order the page list in
changeCategory()instead of callingsortItems().Sorting a directory listing of ASCII file names does not need locale-aware
collation, and doing it here means opening the GUI no longer depends on ICU
data being findable. The visible order is preserved: entries are compared
case-insensitively with a case-sensitive tiebreak so the ordering is total.
Scope
This removes OpenROAD's only unconditional ICU dependency at GUI startup; it
does not fix the underlying
icudefect, which is being reported upstream. Anyother collated sort in an installed GUI build -- for example clicking a header
on the sortable timing tables, or a file dialog's
QFileSystemModel-- wouldstill reach the same unchecked pointer. Those paths were not observed to fire
during headless image generation, but they are not guarded by this change. The
alternative, a
single_version_overridepatchingicu'sputil.cpp, fixes itfor all consumers and can be added if preferred.
Note that every
icuversion currently in the BCR carries this code, including76.1.bcr.4(byte-identical patch) and the newest78.2.bcr.2, so a versionbump is not a workaround.
Verification
Built
--stamp //:openroad-qt; ICU left unpatched. All runs withRUNFILES_DIRand
RUNFILES_MANIFEST_FILEunset.RUNFILES_DIRRUNFILES_DIR/manifestRow 2 is the no-regression check: where the runfiles tree really is present the
.datis still found. After the change noQCollatoris constructed at all, sothe
GUI-0076 Could not create collatorwarning no longer appears either.Real design data, using SiliconCompiler's own image call (
read_db, thengui::show "save_image -resolution <sc_image_resolution 1000> -area {...}") onthe ODB from a
floorplan.initstep: before, SIGSEGV and no image; after,exit 0 and a valid 1099x1099 PNG.
bazel test //src/gui/...passes (3/3). Those tests are man-page and messageconsistency checks and do not exercise
HelpWidget, so they confirm nothingregressed rather than covering the change; there is no C++ unit-test harness
under
src/gui/testto add aHelpWidgettest to.🤖 Generated with Claude Code
Type of Change
Impact
Verification
./etc/Build.sh).