Fix Stack Overflow problem by moving large arrays from the stack to the heap #410
Conversation
Fixes needed to generate a clean build on Win32 platforms (all tests passed).
netcdf_eightbyte.F90 and netcdf_expanded.F90 were generated with M4. Two changes repeated in numerous functions. defaultIntArray is allocated rather than placed on the stack. Added shapeValues variable to eliminate temporary stack usage by "reshape" intrinsic.
fortran/gen.m4
Outdated
There was a problem hiding this comment.
I moved the EightByteInt functions to the bottom of gen.m4. That allows the generated file to be split into two pieces to form "netcdf_expanded.F90" and "netcdf_eightbyte.F90".
fortran/netcdf4_eightbyte.F90
Outdated
There was a problem hiding this comment.
Two changes repeated seven times. Moved "defaultIntArray" and "defaultInt8Array" off of the stack and into the heap. Only one is allocated as needed. Fixed the "reshape" call to avoid an extra stack allocation of the "values" array.
fortran/nveaget48.m4
Outdated
There was a problem hiding this comment.
This function generator contains two changes: Move "defaultIntArray" from the stack to the heap. Fix the "reshape" call to avoid an extra copy of "values" on the stack.
nf03_test4/f90tst_vars4.F90
Outdated
There was a problem hiding this comment.
Even though "data_in" and "data_out" are probably in a static section rather than the stack, I decided to move these two arrays to the heap.
| @@ -37,7 +37,9 @@ IF(NOT NETCDF_C_LIBRARY) | |||
| ENDIF() | |||
|
|
|||
| # Need a copy of ref_fills.nc for ftest | |||
| execute_process(COMMAND cp ${CMAKE_CURRENT_SOURCE_DIR}/ref_fills.nc | |||
There was a problem hiding this comment.
The "cp" command is not available to Windows. I chose CMAKE commands instead. "ftest" fails because the file is missing from the build directory.
| @@ -484,6 +491,7 @@ IF (NETCDF_C_INCLUDE_DIR) | |||
| string(REGEX MATCH "[01]" USE_NETCDF4 "${macrodef}") | |||
| IF (USE_NETCDF4) | |||
| MESSAGE(STATUS "Whether NetCDF-C built with HDF5 enabled: yes") | |||
| FIND_PACKAGE(HDF5 COMPONENTS C HL REQUIRED) | |||
There was a problem hiding this comment.
The netcdf-c cmake file "netCDFTargets.cmake" contains two HDF5 aliases. They do not resolve unless I use FIND_PACKAGE to locate HDF5:
INTERFACE_LINK_LIBRARIES "hdf5-shared;hdf5_hl-shared; ..."
Roy-KC
changed the title
|
I renamed the PR because the defect is not Windows specific. It is bad practice to place large arrays on the stack. Windows platform uncovered the defect because it has a smaller stack size. Since the PR was posted a while ago, the changes are now marked "Outdated" (but they are still relevant). |
|
Thank you; taking a look now, I appreciate your patience! |
Fixes needed to generate a clean build on Win32 platforms (all tests passed).
nf03_test4/f90tst_vars4.F90 failed with a "Stack Overflow" error. The Stack size on Windows is only 1 Mb. Several copies of the data array of size 655k end up on the stack causing the crash. This test actually uncovers a larger problem with an overloaded family of functions (see netcdf_expanded.F90 line 1950) where a large array is placed on the stack (defaultIntArray). This problem is compounded with the following call to RESHAPE:
values(COLONS) = reshape(defaultIntArray(:), shape(values))
Because the RHS contains "values", the compiler chooses to create a temporary array (also on the stack). I made changes to the M4 files to correct the problem. I used M4 to generate "netcdf_expanded.F90" and "netcdf_eightbyte.F90". I hand-edited "netcdf4_eightbyte.F90" because I did not find a way to auto-generate the file.
The affected functions can easily be located by searching for "reshape".
I'll be happy to answer specific questions. Thank you for considering the PR.