Skip to content

fix(geomodel): hide std::__format exports via linker version script#17

Merged
olantwin merged 1 commit into
mainfrom
fix/geomodel-version-script
Jun 5, 2026
Merged

fix(geomodel): hide std::__format exports via linker version script#17
olantwin merged 1 commit into
mainfrom
fix/geomodel-version-script

Conversation

@olantwin

@olantwin olantwin commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a linker version script that marks every _ZNSt8__format* symbol as local in geomodel's shared libraries, passed via CMAKE_SHARED_LINKER_FLAGS.
  • Bumps recipes/geomodel/recipe.yaml build.number to 3.

Why

Round 1 (#15) rebuilt geomodel against the current conda-forge cxx-compiler (no source change, only build.number). Round 2 (#16) added -fvisibility-inlines-hidden. Neither removed the four weak std::__format::_Sink_iter<char> template instantiations that libGeoModelXml, libGeoModelWrite, libGeoModelRead, and libGeoModelDBManager keep exporting:

_ZNSt8__format14__write_padded…
_ZNSt8__format15__write_escaped…
_ZNSt8__format22__write_padded_as_spec…
_ZNSt8__format23__write_escaped_unicode…

libstdc++.so.6 does not export strong overrides, so these weak duplicates remain the only definitions visible to the dynamic linker — and subtle libstdcxx-devel header drift between the geomodel build host and downstream consumers crashes build_geometry inside Gmx2Geo on GitHub Actions (ShipSoft/Geometry#26) while passing on developer machines. -fvisibility-inlines-hidden doesn't affect these because they're standalone template instantiations, not class member inlines; objcopy --localize-symbol/--strip-symbol on the built .sos leaves the .dynsym entries untouched (verified with readelf -W --dyn-syms before/after).

A linker version script applied at link time is the standard way to control DSO exports. Confirmed locally with a minimal extern "C" std::string fmt_it(int i) { return std::format("v={}", i); } test: 81 weak _ZNSt8__format exports drops to 0 with -Wl,--version-script={ local: _ZNSt8__format*; };. Each consumer (libGeoModelXml's own internal calls, build_geometry's own calls) keeps its translation-unit-local instantiation compiled against its own contemporaneous libstdc++ headers — no cross-library ABI dependency.

Test plan

  • CI build job succeeds for the geomodel/shipgeometry/shipgeometryservice/aegir tier
  • After merge, post-merge run uploads geomodel 6.27.0 hb0f4dca_3 to channel ship
  • nm -D reports 0 weak _ZNSt8__format exports in each of libGeoModelXml/Write/Read/DBManager.so.6
  • pixi run test in Geometry/ passes locally, and PR build: rebuild geant4-dependent recipes against geant4 11.4 #26's GitHub Actions run goes green on ubuntu-latest

Summary by CodeRabbit

  • Chores
    • Updated build configuration to optimize library symbol handling and incremented package version.

Round 2 (#16) added -fvisibility-inlines-hidden, but that flag does not
touch standalone template instantiations like
std::__format::__write_padded<…> — they kept being emitted as weak
globals visible in the dynamic symbol table of libGeoModelXml/Write/Read/
DBManager (4 each, verified with `nm -D` on hb0f4dca_2). objcopy
--localize-symbol / --strip-symbol against the produced .so files
also failed to clear them (no observable change in .dynsym), so
post-build patching is not viable.

Pass `-Wl,--version-script=hide-std-format.ver` via
CMAKE_SHARED_LINKER_FLAGS instead. The script marks every
`_ZNSt8__format*` symbol as local on the linker's output, while
leaving the rest of geomodel's API exported. Verified locally with a
minimal `std::format` consumer: 81 weak `_ZNSt8__format` exports drops
to 0 once the version script is in the link line.

Build number bumped to 3 so rattler-build emits a fresh artifact.
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e1c54716-ccfd-43bc-bd8a-b6fb77e538c6

📥 Commits

Reviewing files that changed from the base of the PR and between 5062c4e and 508e2fb.

📒 Files selected for processing (2)
  • recipes/geomodel/build.sh
  • recipes/geomodel/recipe.yaml

📝 Walkthrough

Walkthrough

This PR modifies the geomodel conda recipe build configuration to enforce C++ symbol visibility. The build script now generates a linker version script that marks std::__format* symbols as local, passed to CMake to prevent weak template duplicates from being exposed in shared libraries. The recipe build number is incremented to reflect this rebuild.

Changes

Symbol Visibility Enforcement

Layer / File(s) Summary
Linker version script generation and CMake integration
recipes/geomodel/build.sh
Build script generates hide-std-format.ver to localize std::__format* symbols and integrates it into CMake via CMAKE_SHARED_LINKER_FLAGS, ensuring weak format template instantiations are hidden at link/load time.
Build number increment
recipes/geomodel/recipe.yaml
Recipe metadata build number incremented from 2 to 3 to reflect the rebuild with updated linker configuration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: hiding std::__format symbols via a linker version script in the geomodel build process.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/geomodel-version-script

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@olantwin olantwin merged commit 5cc699a into main Jun 5, 2026
2 checks passed
@olantwin olantwin deleted the fix/geomodel-version-script branch June 5, 2026 12:01
olantwin added a commit to ShipSoft/Geometry that referenced this pull request Jun 5, 2026
ShipSoft/ship-conda-recipes#17 added a linker version script hiding
std::__format weak template instantiations from libGeoModelXml/Write/
Read/DBManager exports. Local symbol audit now reports 0 weak
_ZNSt8__format symbols in those libraries (was 4 each on build 2),
and the full test suite — including OverlapCheck — passes in this
environment.
olantwin added a commit that referenced this pull request Jun 5, 2026
PR #17 added `-Wl,--version-script={local: _ZNSt8__format*;}` which
hid `_ZN…`-mangled function symbols. Local audit confirmed 0 weak
`_ZN` exports per geomodel lib. But CI debug PR (ShipSoft/Geometry#29)
captured a backtrace showing the crash is still in libGeoModelWrite:

  Program received signal SIGSEGV
  #0 std::__format::_Sink<char>::_M_write(…) from libGeoModelWrite.so.6
  #1 std::__format::_Formatting_scanner<…>::_M_on_chars(…) from libGeoModelWrite.so.6
  #2 std::__format::_Scanner<char>::_M_scan(…) at <format>:3942
  …
  #7 SHiPGeometry::CalorimeterFactory::buildStack(…) at CalorimeterFactory.cpp:184

`nm -D libGeoModelWrite.so.6 | grep __format` still shows 15 entries
(all `V` vague-linkage) for vtables (`_ZTV…`), typeinfo (`_ZTI…`), and
typeinfo names (`_ZTS…`) of _Sink<char>, _Buf_sink, _Fixedbuf_sink,
_Seq_sink<string>, _Scanner, _Formatting_scanner. Those are not matched
by `_ZNSt8__format*` (they start with `_ZTV`/`_ZTI`/`_ZTS`, not `_ZN`),
so build_geometry's `_Seq_sink<string>` constructor still binds its
vptr to libGeoModelWrite's vtable, and the virtual call lands in
libGeoModelWrite's compiled-with-different-headers `_M_write`.

Broaden the version script pattern to `*NSt8__format*` so vtables,
typeinfo, typeinfo names, and any future nested std::__format symbols
all get localized. Bump build.number to 4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant