Skip to content

gui: avoid call to sortItems since that triggers from bazel segfault - #11203

Merged
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
gadfort:gui-bazel-segfault
Aug 21, 2026
Merged

gui: avoid call to sortItems since that triggers from bazel segfault#11203
maliberty merged 1 commit into
The-OpenROAD-Project:masterfrom
gadfort:gui-bazel-segfault

Conversation

@gadfort

@gadfort gadfort commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Problem

A GUI-enabled openroad that has been installed dies with SIGSEGV the moment
gui::show runs, on any design including none at all. This kills the
image-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:

$ printf 'gui::show "exit" false\n' > /tmp/t.tcl
$ QT_QPA_PLATFORM=offscreen openroad -no_init -exit /tmp/t.tcl
Signal 11 received
$ echo $?
139

Cause

HelpWidget::init() adds the help categories to a QComboBox. Qt auto-selects
row 0 as soon as the first item lands, which fires changeCategory(), which
called help_list_->sortItems(). That sort compares through QCollator, whose
constructor loads ICU locale data:

HelpWidget::changeCategory -> QListModel::sort -> QListWidgetItem::operator<
  -> QAbstractItemModelPrivate::variantLessThan -> QCollator::defaultCompare
  -> QCollator::QCollator() -> ucol_open -> CollationRoot::load
  -> doOpenChoice -> u_getDataDirectory
  -> rules_cc::cc::runfiles::Runfiles::Rlocation   <-- SIGSEGV

The fault is in the Bazel Central Registry overlay for icu, which patches
u_getDataDirectory() to find the packaged icudt76l.dat through the runfiles
tree using three unchecked calls:

std::unique_ptr<Runfiles> runfiles(Runfiles::Create("", BAZEL_CURRENT_REPOSITORY));
std::string dat_path = runfiles->Rlocation(ICU_DATA_DIR_BAZEL);
path = dat_path.c_str();

Runfiles::Create() is documented to return nullptr on error, and it fails
whenever no runfiles tree is reachable; the next line dereferences it. Since
argv0 is passed as "", rules_cc skips its <argv0>.runfiles fallback, so
only $RUNFILES_DIR and $RUNFILES_MANIFEST_FILE are 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.sh arranges both: the man pages
are installed (line 64), so HelpWidget::init() gets past its path validation;
and openroad.runfiles is deliberately removed after unpacking (lines 47-58).
CLI builds are unaffected -- icu reaches the build only via qt-bazel, so
//:openroad's runfiles contain no icu_dat.

Fix

Order the page list in changeCategory() instead of calling sortItems().
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 icu defect, which is being reported upstream. Any
other collated sort in an installed GUI build -- for example clicking a header
on the sortable timing tables, or a file dialog's QFileSystemModel -- would
still 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_override patching icu's putil.cpp, fixes it
for all consumers and can be added if preferred.

Note that every icu version currently in the BCR carries this code, including
76.1.bcr.4 (byte-identical patch) and the newest 78.2.bcr.2, so a version
bump is not a workaround.

Verification

Built --stamp //:openroad-qt; ICU left unpatched. All runs with RUNFILES_DIR
and RUNFILES_MANIFEST_FILE unset.

case before after
man pages installed SIGSEGV 139 exit 0
man pages installed, valid RUNFILES_DIR collator built collator built
man pages installed, bogus RUNFILES_DIR/manifest GUI-0076 warning exit 0
no man pages exit 0 exit 0

Row 2 is the no-regression check: where the runfiles tree really is present the
.dat is still found. After the change no QCollator is constructed at all, so
the GUI-0076 Could not create collator warning no longer appears either.

Real design data, using SiliconCompiler's own image call (read_db, then
gui::show "save_image -resolution <sc_image_resolution 1000> -area {...}") on
the ODB from a floorplan.init step: 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 message
consistency checks and do not exercise HelpWidget, so they confirm nothing
regressed rather than covering the change; there is no C++ unit-test harness
under src/gui/test to add a HelpWidget test to.

🤖 Generated with Claude Code

Type of Change

  • Bug fix

Impact

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines.
  • I have signed my commits (DCO).

Signed-off-by: Peter Gadfort <gadfort@zeroasic.com>
@gadfort
gadfort requested a review from a team as a code owner August 21, 2026 15:42
@gadfort
gadfort requested a review from maliberty August 21, 2026 15:42

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +116 to +117
pages.emplace_back(QString::fromStdString(path.stem()),
QString::fromStdString(path));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
pages.emplace_back(QString::fromStdString(path.stem()),
QString::fromStdString(path));
pages.emplace_back(QString::fromStdString(path.stem().string()),
QString::fromStdString(path.string()));

@maliberty
maliberty merged commit 110aaff into The-OpenROAD-Project:master Aug 21, 2026
17 checks passed
@gadfort
gadfort deleted the gui-bazel-segfault branch August 22, 2026 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants