Repository navigation
fix: keep large arrays off the stack on Windows GNU builds - #3
Merged
Merged
Conversation
With OpenMP enabled, gfortran places fixed-size local arrays on the stack (-fopenmp implies -frecursive). The taut-string case of the modal test kept 1.7 MB of dense matrices and mode shapes there, within 0.25 MB of the 2 MB MinGW default reserve, and stopped with a stack overflow on some Windows CI runners. Allocate it, and the other test arrays larger than 64 KB (modal, snap-load, and torsion tests), on the heap.
CD_AGG_Init_From_Deck, the C API deck initialisation, and the driver's mixed route each held one or two aggregate bundles (about 37 KB and 33 KB) as stack locals: frames of 108 KB, 74 KB, and 83 KB. Initialisation runs on the thread of the calling host, whose stack size CableDyn does not choose, so allocate them instead. This lowers the deepest stack of a C API run from about 345 KB to 250 KB. The per-step path is unchanged and still does not allocate.
…tack Link every executable of a MinGW-w64 gfortran build with a 64 MiB stack reserve instead of the 2 MiB default, since the stack depth of the OpenBLAS kernels differs between CPUs. A shared library loaded by a host such as python.exe cannot rely on this reserve, so new tests bound the stack the library needs: - stack_c_api: a 2048-element finite-EI cable (12294 DOFs), an EI=0 + finite-EI aggregate, and an EI=0 mooring are initialised, stepped, and evaluated through the C API on a thread with a 1 MiB stack (every platform). - stack_modal and stack_driver_large_deck (GNU Windows): the modal test and the driver relinked with a 1 MiB reserve; the driver runs the 2048-element cable. stack_modal fails with the former modal test. - stack_reserve: the reserve each of these links received. Document the reserve and the stack rule in DEVELOPMENT.md.
… inputs The stack_reserve test needed objdump and was silently not registered when CMake did not find it. It now reads SizeOfStackReserve from the PE optional header with CMake alone, and stack_reserve_mismatch checks that the reader reports a reserve that differs from the expected one. test_c_api_stack rejects malformed stack sizes and coupling steps, rounds a POSIX thread stack up to whole pages, and fails when the wait for the Win32 thread fails.
…lease The Visual Studio project that builds the CableDyn adapter into the static openfast.exe set no heap-arrays option, so with IFX its run-time-sized arrays and temporaries went on the openfast.exe stack. Every configuration now sets HeapArrays="1024", which Visual Studio passes as /heap-arrays1024, the same threshold as the /heap-arrays:1024 of the static CMake build of the core.
gfortran 16.2 reported 29 -Wuninitialized warnings that 15.2 does not. Each names the bounds, offset, or span of an unallocated allocatable component of a default-initialised local (CD_HFMF_ModuleType or CD_HermiteCableDynType): the optimiser copies these never-set descriptor fields when it rewrites the default initialisation, and the program never reads them. These locals, in the still-water friction reference of the recursive Hermite cable builder and in six tests, are now ALLOCATABLE, which leaves results unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an intermittent stack overflow on Windows GNU builds. The
modaltest kept about 1.7 MB of fixed-size local arrays on the stack against the 2 MB MinGW default reserve, so on some CI runners the extra stack used by processor-specific OpenBLAS kernels tipped it over (0xC00000FD, reported by CTest as a segmentation fault). Linux builds, with an 8 MB stack, were not affected.Changes
test_modal,test_snap_loadand the torsion tests).openfast.exe: the Visual Studio project template for the CableDyn adapter now compiles with heap arrays, matching the core library. Verified with a full release build and the coupled smoke cases.-Wuninitializedwarnings removed by making the affected locals allocatable; no behaviour change.DEVELOPMENT.md, a note in the OpenFAST integration README, and a changelog entry.Regression tests (label
stack)stack_c_api: runs a 2048-element cable (12,294 unknowns), the IEA mixed deck and the VolturnUS mooring through the C API on a thread with a 1 MiB stack, on every platform.stack_modal,stack_driver_large_deck(Windows GNU): the modal test and the driver relinked with a 1 MiB reserve. The oldtest_modalfails this exactly as CI did.stack_reserve/stack_reserve_mismatch: read the reserve from the PE header with CMake alone.Checks
dev.stack_c_apiruns for the first time on Linux CI.