Skip to content

Commit 26ff25c

Browse files
committed
fix: tinyobjloader ODR violation unmasked by tonight's CI-red fixes
Real windows-2022 + Clang ASan+UBSan CI evidence landed after tonight's 6 CI-red fixes: mc3togltf.exe now starts on Windows and the 3 load_policy tests pass, confirming those fixes work. But clearing the mc3_json_document_budget timeout let the Linux sanitizer job's standalone-component loop reach mc3togltf's own tests for the first time ever, and ~67 of 75 crashed instantly with an AddressSanitizer odr-violation on tinyobj::MaterialFileReader's typeinfo. Root cause: mc3togltf/src/MeshBuilder.cpp defines TINYOBJLOADER_IMPLEMENTATION and #includes tiny_obj_loader.h directly, compiling its own copy of tinyobjloader into mc3togltf_lib -- while mc3togltf_lib (and the root editor target) ALSO linked the separately-compiled `tinyobjloader` CMake target, which builds as a shared library for the same Manifold BUILD_SHARED_LIBS=ON default already identified as the Windows DLL-staging bug. Two live definitions of the same class/vtable in one process is genuine undefined behavior, not an ASan nitpick -- it was always latent, just never reached in CI before (the loop always died earlier at mc3's own timeout). Fixed by dropping the tinyobjloader *link* dependency everywhere (mc3togltf_lib, both branches of the root editor target's target_link_libraries, and the release-artifact DLL/shared-library install loop) -- only the include directory is needed since MeshBuilder.cpp already provides the sole implementation. Verified: standalone mc3togltf rebuilt under Clang ASan+UBSan matching CI's exact flags -- 75/78 pass (up from 8/78), remaining 3 are the already-known Blender/numpy gap. Full root MeshCraft editor still links/runs (--version works), and OBJ import specifically (mc3togltf_obj_material_import/_obj_robustness/_large_obj_stress) still passes end to end.
1 parent f72f90b commit 26ff25c

3 files changed

Lines changed: 73 additions & 16 deletions

File tree

CMakeLists.txt

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -467,10 +467,14 @@ if(NOT EMSCRIPTEN AND NOT ANDROID)
467467
endif()
468468
endif()
469469

470-
# mc3togltf is linked against these shared third-party libraries in the
471-
# default desktop build. Ship their runtime files in the same release
470+
# mc3togltf is linked against this shared third-party library in the
471+
# default desktop build. Ship its runtime files in the same release
472472
# component so the CLI archive is usable outside the build tree.
473-
foreach(MESHCRAFT_CLI_RUNTIME_TARGET IN ITEMS manifold tinyobjloader)
473+
# tinyobjloader is deliberately NOT listed here: mc3togltf_lib only ever uses
474+
# it as a single-header library (see mc3togltf/CMakeLists.txt's
475+
# TINYOBJLOADER_IMPLEMENTATION note) and no longer links its separately
476+
# compiled target, so there is no runtime tinyobjloader artifact to ship.
477+
foreach(MESHCRAFT_CLI_RUNTIME_TARGET IN ITEMS manifold)
474478
if(TARGET ${MESHCRAFT_CLI_RUNTIME_TARGET})
475479
install(TARGETS ${MESHCRAFT_CLI_RUNTIME_TARGET}
476480
ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}" COMPONENT release
@@ -655,14 +659,13 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang" AND NOT WIN32 AND NOT ANDROID)
655659
mc3togltf_lib
656660
imgui
657661
manifold
658-
tinyobjloader
659662
lua54
660663
sol2::sol2)
661664
if(SQLite3_FOUND)
662665
target_link_libraries(${_target} PRIVATE ${SQLite3_LIBRARIES})
663666
endif()
664667
else()
665-
target_link_libraries(${_target} PRIVATE CNA ${_cna_backend_target} SHARP_RUNTIME Mc3 Mcb mc3togltf_lib imgui manifold tinyobjloader lua54 sol2::sol2)
668+
target_link_libraries(${_target} PRIVATE CNA ${_cna_backend_target} SHARP_RUNTIME Mc3 Mcb mc3togltf_lib imgui manifold lua54 sol2::sol2)
666669
if(SQLite3_FOUND)
667670
target_link_libraries(${_target} PRIVATE ${SQLite3_LIBRARIES})
668671
endif()

NEXT.md

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,51 @@ place. Progress, each its own commit:
227227
mechanism itself is standard CMake, not something this project invented.
228228
**All 6 of tonight's deferred CI-red regressions are now addressed.**
229229

230+
**Real CI evidence landed after pushing the above.** Confirmed working on an
231+
actual `windows-2022` runner: `mc3togltf.exe` now starts (no more
232+
`STATUS_DLL_NOT_FOUND`) and `mc3_load_policy`/`mc3_json_load_policy`/
233+
`mcb_load_policy` all pass. But clearing those earlier blockers let both the
234+
Windows and Linux sanitizer jobs run further than ever before, surfacing 2
235+
new, previously-unreachable bugs (same "fixing one bug unmasks the next"
236+
pattern as this whole session):
237+
238+
- **Linux Clang ASan+UBSan: ~67 of 75 `mc3togltf_*` tests crashed at process
239+
startup** with an AddressSanitizer odr-violation on
240+
`typeinfo name for tinyobj::MaterialFileReader` between
241+
`mc3togltf/src/MeshBuilder.cpp` and `libtinyobjloader.so.2`. Root cause:
242+
`MeshBuilder.cpp` defines `TINYOBJLOADER_IMPLEMENTATION` and `#include`s
243+
the header directly (compiling its own copy into `mc3togltf_lib`), while
244+
`mc3togltf_lib` ALSO linked the separately-compiled `tinyobjloader` CMake
245+
target — which builds as a shared library for the same
246+
Manifold-`BUILD_SHARED_LIBS`-default reason as the Windows DLL bug above.
247+
Two live definitions of the same class/vtable in one process is genuine
248+
undefined behavior, not just an ASan nitpick. This was always latent but
249+
never reached in CI before tonight: the standalone-component loop always
250+
died earlier at `mc3`'s `mc3_json_document_budget` timeout (CI-red #6,
251+
now fixed), before ever building/running `mc3togltf`'s own tests.
252+
Fixed by dropping the `tinyobjloader` *link* dependency everywhere
253+
(`mc3togltf/CMakeLists.txt`'s `mc3togltf_lib`, and both branches of the
254+
root `CMakeLists.txt`'s main editor target) — only the include directory
255+
is needed since `MeshBuilder.cpp` already provides the one-and-only
256+
implementation. Also dropped `tinyobjloader` from the root project's
257+
release-artifact DLL/shared-library install loop, since nothing links it
258+
at runtime anymore. Verified: rebuilt the standalone `mc3togltf` component
259+
under Clang ASan+UBSan matching CI's exact flags (`build-sanitize/mc3togltf`)
260+
— 75/78 pass (up from 8/78), the 3 failures are the already-known
261+
Blender/`numpy` gap; the full root `MeshCraft` editor (`cmake-build-debug`)
262+
still links and runs (`--version` works), and OBJ import specifically
263+
(`mc3togltf_obj_material_import`/`_obj_robustness`/`_large_obj_stress`)
264+
still passes, confirming tinyobjloader itself still works correctly
265+
through its single remaining (header-only) code path.
266+
- **Windows: `mc3_roundtrip` still crashes, but for a different, new reason**
267+
than tonight's earlier fix. `terminate() after throwing
268+
MeshCraft::Mc3::AtomicFinalizeError`: `Mc3::writeFileAtomically()`'s
269+
finalize `rename()` step fails with "Input/output error" for a path
270+
containing non-ASCII characters (`mc3_rt_čeština_日本.mc3.xml`, the
271+
STAB-0555/0556 UTF-8-filename case, reached via `roundtripAt()`). This is
272+
a bug in the `SYS-W9-06` atomic-write primitive itself, unrelated to the
273+
ifstream/`remove()` bug fixed above — investigating next.
274+
230275
## Known release blockers and decisions
231276

232277
| Area | Live state |

mc3togltf/CMakeLists.txt

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -100,16 +100,15 @@ if(NOT tinyobjloader_POPULATED)
100100
endif()
101101

102102
# Windows STATUS_DLL_NOT_FOUND (part of tonight's CI-red survey): Manifold's
103-
# own CMakeLists.txt defaults BUILD_SHARED_LIBS to ON, and tinyobjloader's
104-
# add_library() call has no explicit STATIC/SHARED, so it inherits that same
105-
# global cache value -- both build as DLLs on Windows. Linux/macOS need no
106-
# fix: CMake adds build-tree RPATH entries to every executable pointing at
107-
# its shared-library dependencies automatically, and Windows PE executables
108-
# have no equivalent mechanism (the loader only searches the exe's own
109-
# directory, CWD, System32, and PATH -- nothing under _deps/). Copy each
110-
# affected executable's transitive DLL dependencies next to it after linking;
111-
# $<TARGET_RUNTIME_DLLS:...> (CMake 3.21+, the minimum this project already
112-
# requires) resolves to nothing on non-Windows, so this is a no-op there.
103+
# own CMakeLists.txt defaults BUILD_SHARED_LIBS to ON, so it builds as a DLL
104+
# on Windows. Linux/macOS need no fix: CMake adds build-tree RPATH entries to
105+
# every executable pointing at its shared-library dependencies automatically,
106+
# and Windows PE executables have no equivalent mechanism (the loader only
107+
# searches the exe's own directory, CWD, System32, and PATH -- nothing under
108+
# _deps/). Copy each affected executable's transitive DLL dependencies next
109+
# to it after linking; $<TARGET_RUNTIME_DLLS:...> (CMake 3.21+, the minimum
110+
# this project already requires) resolves to nothing on non-Windows, so this
111+
# is a no-op there.
113112
function(mc3togltf_stage_runtime_dlls target)
114113
if(WIN32)
115114
add_custom_command(TARGET ${target} POST_BUILD
@@ -146,7 +145,17 @@ target_include_directories(mc3togltf_lib SYSTEM PUBLIC
146145
${nanosvg_SOURCE_DIR}/src
147146
)
148147

149-
target_link_libraries(mc3togltf_lib PUBLIC Mc3 tinyobjloader manifold)
148+
# tinyobjloader is used as a single-header library here (MeshBuilder.cpp
149+
# defines TINYOBJLOADER_IMPLEMENTATION and #includes it directly), so only
150+
# its include dir (added above) is needed -- deliberately NOT linking the
151+
# separately-compiled `tinyobjloader` CMake target too. That target also
152+
# compiles tiny_obj_loader.cc into its own library (shared, by inheriting
153+
# Manifold's BUILD_SHARED_LIBS=ON default), so linking both would give the
154+
# final binary two live definitions of the same class/vtable: a genuine ODR
155+
# violation, caught by AddressSanitizer's odr-violation detector (crashes
156+
# every mc3togltf sanitizer test at process startup, before any test code
157+
# even runs) and undefined behavior on any build, sanitized or not.
158+
target_link_libraries(mc3togltf_lib PUBLIC Mc3 manifold)
150159

151160
if(MSVC)
152161
set(MESHCRAFT_STANDALONE_WARNING_FLAGS /W4)

0 commit comments

Comments
 (0)