Repository navigation
Pin softfloat 3.0.0#3: thread-local rounding and exception state - #701
Conversation
f71efd5 to
b7eced1
Compare
| add_compile_definitions(BOOST_TEST_DEFAULTS_TO_CORE_DUMP) | ||
| # softfloat's legacy float32_t/float64_t aliases collide with arm_neon.h, which Boost.Unordered pulls in on arm64; | ||
| # nothing here uses them. | ||
| add_compile_definitions(SOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASES) |
There was a problem hiding this comment.
Medium: This definition is directory-scoped, so it does not reach consumers of the generated SysioChain/SysioTester interface. SysioTester.cmake.in links libsoftfloat as a raw archive and exports the SoftFloat include paths without this compile definition. On the documented Apple-Silicon wire-cdt integration path, tester.hpp includes softfloat_types.h before Boost.Unordered pulls in arm_neon.h, reproducing typedef redefinitions for float16_t, float32_t, and float64_t. I reproduced the failure with a Clang 18 AArch64 syntax probe; it passes when this macro is defined. Please export the definition on the generated SysioChain interface, while retaining the in-tree definition as needed.
There was a problem hiding this comment.
Exported in 4af9ddb: SysioChain now carries SOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASES and, since it links the archive directly rather than the port's package, also THREAD_LOCAL=__thread, the storage class the archive was built with. Checked with a build-tree find_package(sysio) consumer: both land in INTERFACE_COMPILE_DEFINITIONS. The tree-wide definition stays.
| { | ||
| "name": "softfloat", | ||
| "version": "3.0.0#2" | ||
| "version": "3.0.0#3" |
There was a problem hiding this comment.
Medium (test coverage): This version bump changes the four SoftFloat state variables from process-global to thread-local, but the current wire-sysio golden tests are serial and never assert rounding or exception-state isolation. The dependency contains a deterministic TLS test, but the registry port configures it with BUILD_TESTING=OFF, so PR CI does not exercise this behavior. Please add a two-thread regression test that assigns distinct rounding modes and exception flags and verifies they do not leak; it should fail with 3.0.0#2 and pass with #3.
There was a problem hiding this comment.
Added unittests/softfloat_thread_local_tests in 4af9ddb: the calling thread sets softfloat_round_max and raises softfloat_flag_infinite, a worker must start from the defaults, set softfloat_round_minMag and raise softfloat_flag_invalid, and neither side may see the other's state. Against the #2 archive (no THREAD_LOCAL, process-global state) all five checks fail at runtime (the worker sees round_max and infinite, the caller ends with minMag and invalid | infinite); against #3 it passes, in this tree under --sys-vm, --sys-vm-jit and --sys-vm-oc.
b7eced1 to
4af9ddb
Compare
huangminghuang
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The two earlier findings are addressed, and I found no remaining actionable issues.
Moves the wire registry baseline to abed0cc1 (the wire-vcpkg-registry#22 merge) and the softfloat override to 3.0.0#3, which builds Wire-Network/berkeley-softfloat-3@e9bb379 (its PR #5): THREAD_LOCAL is now a PUBLIC definition of the softfloat target, so softfloat_roundingMode, softfloat_detectTininess, softfloat_exceptionFlags and extF80_roundingPrecision are per thread instead of process globals shared by the read-only transaction threads and the sys-vm runtimes. Ports gofractally/eos-vm#24 and #25 to Wire's vcpkg-built softfloat. Also defines SOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASES tree-wide and, with THREAD_LOCAL, on the exported SysioChain interface (which links the archive directly): since berkeley-softfloat-3@67af45c the port no longer forces the alias disable, and the legacy float32_t/float64_t aliases collide with arm_neon.h, which Boost.Unordered pulls into most chain TUs on arm64. Adds softfloat_thread_local_tests: a worker thread starts from the default rounding mode and flags and leaks nothing back. Change-Id: Id884de24d946846e8802a83c84c369862d4118f0
4af9ddb to
b759074
Compare
huangminghuang
left a comment
There was a problem hiding this comment.
Reviewed the current diff and relevant SoftFloat, vcpkg registry, build/export, and regression-test context. No actionable findings.
Brings in #651, #659 and #701. sysio.epoch.wasm is rebuilt from the merged sources, and every other contract matches its committed wasm and ABI. MAX_OPERATORS_PER_EPOCH keeps the cap of 7 with #659's cost wording, one recorddel and termcheck per member. #659's shared rotation walk now checks the window roster: a member terminated as its group expires is no longer listed.
Builds on Wire-Network/berkeley-softfloat-3#5 and Wire-Network/wire-vcpkg-registry#22, both merged. The registry baseline is
abed0cc1, the #22 merge commit, whose tree is identical to the2a0f6fc4head this was verified against. The pendingwire-sys-vmbump (Wire-Network/wire-vcpkg-registry#23, for Wire-Network/eos-vm#14) merges cleanly on top ofabed0cc1; #703 re-pins to the registry commit carrying both.Berkeley SoftFloat's rounding mode, tininess mode, exception flags and extF80 precision were process globals: the vcpkg port passed no
THREAD_LOCAL, so every read-only transaction thread and every sys-vm / sys-vm-jit / sys-vm-oc execution raced on them. gofractally/eos-vm#24 and gofractally/eos-vm#25 fixed this upstream inexternal/softfloat.cmake, which Wire does not build. The port now exportsTHREAD_LOCAL(__threadon GCC and Clang for C and C++,__declspec(thread)on MSVC);sysio_chainlinkssoftfloat::softfloatPUBLIC, so every chain consumer's compile line carries the define and declares the state with the storage class the library defines it with (the linker rejects a TLS/non-TLS mismatch).__threadis the spellingsoftfloat.hppand wasm-jit'sPlatform.halready use, so the redefinition inwasm_interface.cppstays identical and silent. The pin also carries the softfloat fix for its arm64-only NEON include-order test, which broke the port build on arm64-osx on the first pin. The second macOS failure was in wire-sysio itself (controller.cpp): since berkeley-softfloat-3@67af45c the softfloat target no longer forcesSOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASESon arm64 consumers, and the legacyfloat32_t/float64_t/float16_taliases collide witharm_vector_types.h, which Boost.Unordered'sarm_neon.hinclude brings into most chain TUs. Nothing in wire-sysio or the sys-vm headers uses those aliases, so the root CMake now defines the macro for the whole tree (the x86_64 build is unaffected except for the define on every compile line). The generatedSysioChaininterface (cmake/SysioTester.cmake.in) links the softfloat archive directly and bypasses the port's package, so it now repeats the two definitions the package carries:THREAD_LOCAL=__thread, the storage class the archive was built with, andSOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASES; a build-treefind_package(sysio)consumer sees both onSysioChain.unittests/softfloat_thread_local_tests.cppis the regression test for the pin: the calling thread setssoftfloat_round_maxand raisessoftfloat_flag_infinite, a worker must start fromsoftfloat_round_near_evenwith clear flags, setsoftfloat_round_minMagand raisesoftfloat_flag_invalid, and neither side may see the other's state. Linked against the 3.0.0#2 state (process globals, noTHREAD_LOCAL) all five checks fail: the worker seesround_maxand theinfiniteflag, and the caller ends withminMagandinvalid | infinite; against #3 it passes.Verification (clang 18.1.8, Debug, x64-linux, contracts OFF):
unit_test --run_test=softfloat_thread_local_testsunder--sys-vm,--sys-vm-jit,--sys-vm-oc:*** No errors detected.libsoftfloat.afrom the pinned port (treebca9aee, identical at2a0f6fc4andabed0cc1, fetched from GitHub):softfloat_roundingMode,softfloat_detectTininess,softfloat_exceptionFlags,extF80_roundingPrecisionareTLS GLOBALinreadelf -sW(3.0.0#2 had them as plainOBJECT);nodeopandunit_testreference them asTLS.softfloat-config.cmakeexports a plainTHREAD_LOCAL=__thread; 505 compile lines carry it and 684 carry-DSOFTFLOAT_DISABLE_LEGACY_TYPE_ALIASES. The build has zero warnings.unit_test --run_test=softfloat_golden_testsunder--sys-vm,--sys-vm-jit,--sys-vm-oc:*** No errors detected(50 cases each).unit_test --run_test=wasm_part1_tests,wasm_part2_tests,wasm_part3_testsunder--sys-vm,--sys-vm-jit,--sys-vm-oc:*** No errors detected(52 cases each).python3 tests/read_only_trx_test.py -p 2 -n 3 --read-only-threads 2:Test succeeded.Linux CI was green on both earlier pins (ubuntu24, ubsan, asan, gcc, asserton); the macOS job is the check for the arm64 alias collision, which cannot be built here.