ENH: Configure the ObjectFactory autoload symbol name - #6787
Conversation
3c05614 to
1783a9a
Compare
1783a9a to
72db5ec
Compare
dzenanz
left a comment
There was a problem hiding this comment.
How would this be used by Slicer? Where does #define ITK_LOAD_FUNCTION_NAME slicer_itkLoad needs to be put?
72db5ec to
0277890
Compare
|
Nowhere in source — that's the change from the fork patch. It's a CMake cache variable now, so Slicer sets it once when configuring ITK: # Slicer/SuperBuild/External_ITK.cmake, alongside the other -DITK_* entries
# in the CMAKE_CACHE_ARGS list (~line 96-147)
-DITK_LOAD_FUNCTION_NAME:STRING=slicer_itkLoadITK then configures that into the generated What Slicer would still need to change (2 lines)One Slicer file declares the entry point literally: // Slicer/Libs/MRML/IDImageIO/itkMRMLIDIOPlugin.h:24
MRMLIDIOPlugin_EXPORT itk::ObjectFactoryBase* itkLoad();
// Slicer/Libs/MRML/IDImageIO/itkMRMLIDIOPlugin.cxx:10
itk::ObjectFactoryBase* itkLoad()Both become That appears to be the only one. I searched the Slicer ecosystem in the forest build testbed for
So no extension defines a factory plugin, and nothing outside that one Slicer pair needs touching. Reading the value back from CMake
set(ITK_LOAD_FUNCTION_NAME "@ITK_LOAD_FUNCTION_NAME@")so Slicer's own CMake (and the extension build system) can read what ITK was actually built with rather than assuming. This is the pattern @blowekamp asked for in the 2022 Discourse thread for the namespace value. The name is also validated at configure time — a value that is not a valid C identifier is a Scope — what this does and does not coverThis handles only the ObjectFactory autoload symbol. It is not the namespace feature. The reason it needs its own mechanism is that $ nm -gU libexternc.so
__ZN3itk2v67cppFuncEv # C++ inside `inline namespace v6` -> mangled, namespaced
_itkLoad # extern "C" -> untouchedVTK hit the same thing and ships So this is one piece of what the fork patch does, upstreamed on its own merits. The broader question — whether ITK supports a configurable namespace at all, and by what mechanism — is #6786, which this PR does not decide. #6786's Phase 2 records how Worth flagging for sequencing: Slicer's current branch computes the name as |
This comment was marked as low quality.
This comment was marked as low quality.
|
Backport to |
0277890 to
a54b6cd
Compare
|
In the current build/configuration of 3D Slicer this change I don't think is needed. See: Slicer/Slicer#8947 for the issue. They disabled adding the itkLoad function to the problematic ImageIO in 3DSlicer. There is still SimpleITK and ITKPython which can try to load libraries and check for the itkLoad symbol. This can cause problems with the versions or build configuration of ITK does not match.With underlying issue reported above it is unclear to me if loading the library causes broad symbol conflict or if just creating an ITK object from the separate build. We should consider making a more robust solution to the underlying problem. Perhaps adding a ITK version or build number automatically to the name to more broadly prevent the issue with loading itkLoad. EDIT: This may just be future work with how to define the function name that is introduced here. |
|
@hjmjohnson Maybe try using this in Slicer before merging? |
|
Ran this through the downstream forest testbed (33 ITK consumers built against Two consumers hardcode The Slicer half is now handled and verified — Where the two consumers hardcode it, and who actually uses the mechanism
A separate sweep for MITK ships the generator but no (BRAINSTools matches were false positives — MATLAB comments about unrelated Suggested follow-ups
None of these block this PR. What the forest verified cleanForest No macro collision anywhere. This PR adds The "34 copies across 17 files" claim checks out exactly. 16 files define Build: ITK End-to-end, in a real Slicer SuperBuild. Slicer was built against an ITK plugin libMRMLIDIOPlugin.dylib exports: _slicer_itkLoad (no _itkLoad)
loader libITKCommon-6.0.1.dylib looks up: slicer_itkLoad (no "itkLoad")The negative controls are the point: had the plugin not followed, it would Caveat worth stating: Slicer defaults to Two notes on the change itselfThis invalidates every ccache entry in the ecosystem. Minor: |
itkMRMLIDIOPlugin exported itkLoad by literal name while ObjectFactoryBase looks the symbol up by the name ITK was configured with. The two agree only while that name is left at its default. An ITK built with a non-default ITK_LOAD_FUNCTION_NAME searches for that symbol and finds nothing here, and LoadLibrariesInPath is silent when a library exports no entry point, so MRMLIDImageIO would simply never register. The fallback is transient, kept only while Slicer supports ITK releases that predate the configured name. It is guarded on the macro because ITK main carries version 6.0.0 both before and after the change, so no ITK_VERSION comparison separates the two; the removal condition is stated in an ITK_VERSION-bearing comment so the shim is found by the same search as the rest of the version-conditional code. Refs: InsightSoftwareConsortium/ITK#6787, InsightSoftwareConsortium/ITK#6786
a54b6cd to
8c79fbb
Compare
|
The version/build-number idea solves a different problem than the one this is aimed at, and I think the two are composable rather than alternatives. The target scenario is three independently-distributed ITK builds coexisting in one process — PyPI On Slicer specifically: you're right that #8947 took the problematic ImageIO out of play, but the forest scan found a second site — Agreed on SimpleITK and ITKPython being the sharper exposure, and I don't think this PR settles it either — it makes the entry-point name configurable, which is a precondition, not the answer. Your framing in the EDIT matches how I'd scope it: this is enabling work with no behavior change today. No harm, no current improvement, one of four names that have to be configurable before the larger change is possible. The four axes, and why the other two matter here
Your point on the other issue about the CMake interface namespace turns out to be on the critical path rather than an aside. The filename axis in particular is load-bearing: three shared libraries all named Which is also why the open question about whether loading causes broad symbol conflict or only affects object creation is worth settling empirically before the mechanism is chosen — I don't think it has been. The target scenario and the axis table are now written up in #6786, along with a status table for the pieces already in flight. |
ObjectFactoryBase looked up the autoload entry point by string literal while plugins defined it by name, so the two agreed only by convention. Both now derive from ITK_LOAD_FUNCTION_NAME, configured once into itkConfigure.h. The name is configurable because extern "C" names carry no C++ namespace, so a build that renames ITK's symbols cannot reach this entry point through the namespace. Setting it in the generated header keeps the loader and every plugin consistent by construction. No behavior change; the default is itkLoad. Co-Authored-By: Bradley Lowekamp <blowekamp@mail.nih.gov>
8c79fbb to
da0658e
Compare
ObjectFactoryBaselooked up the autoload entry point by string literal while plugins define it by name, so the two agreed only by convention. Both now derive fromITK_LOAD_FUNCTION_NAME, configured once intoitkConfigure.h. No behavior change — the default isitkLoad.This is enabling work with no improvement today. It makes one of four names configurable so that independently-distributed ITK builds can eventually coexist in one process — PyPI
and is not decided here.
itkwheels aspy_itk, SimpleITK assimple_itk, Slicer asslicer_itk, possibly built from the same source.extern "C"names carry no C++ namespace, so the autoload entry point needs its own configurable name whichever namespace mechanism ITK adopts. That mechanism question isrelease-5.4backport of this PRReviewers: the plugin-facing code is only reachable with
BUILD_SHARED_LIBS=ON. ITK guards both factory plugin targets behindif(ITK_BUILD_SHARED_LIBS)(Modules/Core/Common/test/CMakeLists.txt:151,Modules/IO/ImageBase/test/CMakeLists.txt:933), so 3 of the 9 changed files are compiled only in a shared build (itkFactoryTestLib.h,itkFactoryTestLib.cxx,itkFileFreeImageIOFactory.cxx) and a static build does not registeritkIOPluginTest. Of the remainder, 2 are CMake and 4 compile in both configurations.The four naming axes, and why this is one of them
Coexisting builds need all four aligned. Two are already configurable on
main:mainITK_NAMESPACEor an ABI inline namespaceITK_LOAD_FUNCTION_NAMEITK_LIBRARY_NAMESPACE(CMakeLists.txt:205)ITK_CUSTOM_LIBRARY_SUFFIX(ITKModuleMacros.cmake:731)ITK_LOAD_FUNCTION_NAMEis defined at the top level besideITK_LIBRARY_NAMESPACE, so all four are settable before any module is configured.The filename axis is not cosmetic: three shared libraries all named
libITKCommon-6.0.soin one process means the dynamic loader keys on SONAME and the first one loaded wins, so the C++ symbols can be perfectly distinct and it still will not work.Why the name is build-configured rather than a header
#ifndefAn earlier revision put an overridable
#ifndef ITK_LOAD_FUNCTION_NAMEinitkObjectFactoryBase.h. That was unsound and has been replaced.The lookup side is compiled into
libITKCommonwhen ITK is built. A downstream consumer defining the macro before including the installed header would change only its own plugin's exported symbol, while the prebuilt loader still searched foritkLoad— the plugin would silently never load. Confirmed against the built library:Routing the value through the generated
itkConfigure.hmakes disagreement structurally impossible: there is one generated header, and the loader and every plugin include it.Why
extern "C"needs its own mechanismextern "C"names carry no C++ namespace, so no namespace scheme reaches this entry point. Demonstrated on one library containing both kinds of symbol:VTK hit the same thing and ships
VTK_ABI_NAMESPACE_MANGLE(x)forGetVTKVersion,signal_handler, and its serialization registrars.Also included:
ITK_STRINGIFYinitkMacro.hThe loader needs the configured token as a string, which requires the two-level stringify idiom —
#does not expand its operand, so a single level yields"ITK_LOAD_FUNCTION_NAME".Rather than adding another local copy, this PR puts
ITK_STRINGIFY/ITK_STRINGIFY_HELPERinitkMacro.h. ITK already contains 34 hand-rolled copies of this idiom across 17 files — 32 named_STRING/TOSTRING, and_STRINGis a reserved identifier ([lex.name]/3.1), so those are undefined behavior as well as duplicates.Migrating the existing 34 is deliberately not in this PR; it is tracked as #6788. This PR adds only the shared definition its own single call site needs.
Verification
macOS 15 arm64, Apple clang via the pixi
cxxenvironment, Release.1.
BUILD_SHARED_LIBS=ON, default — build clean,itkIOPluginTestPASS.2.
BUILD_SHARED_LIBS=ON,-DITK_LOAD_FUNCTION_NAME=slicer_itkLoad— build clean,itkIOPluginTestPASS.Loader and plugin move together; the factory still loads.
3. Validation — a value that is not a valid C identifier fails configure:
4.
BUILD_SHARED_LIBS=OFF— build clean, 4/4ObjectFactorytests PASS.ctest -N -R itkIOPluginTestreportsTotal Tests: 0, confirming the plugin test is absent by ITK's own guard rather than by anything in this change.pre-commit run --all-filespasses on the branch tip.