fix: windows compilation of hash maps tests and increase stack size avoiding segfaults #988
Conversation
jalvesz
changed the title
|
Ideally there would be a way to avoid the issues with the libs, instead of fixing them this way, but it seems to work. |
|
Also this may warrant adding the intel oneapi llvm compiler on windows to the compilers which work, but aren't tested in the readme. |
|
Yes, I agree that ideally this should be managed differently but I'm not aware of a better solution compared to what we found here. It is good that indeed, it enables full compliance on Windows for the tests. |
| @@ -2,6 +2,14 @@ if (NOT TARGET "test-drive::test-drive") | |||
| find_package("test-drive" REQUIRED) | |||
| endif() | |||
|
|
|||
| if(WIN32) | |||
| if(CMAKE_Fortran_COMPILER_ID MATCHES "^Intel") | |||
| add_link_options(/Qoption,link,/STACK:8388608) | |||
There was a problem hiding this comment.
I've looked in the hashmap source file and I can't find reasons more stack size would be needed: there are no static (save)d variables or arrays there, only allocatables.
I'm worried that increasing the stack size may just be hiding other issues with the code that we haven't figured out yet. For example, the test cases include static arrays, maybe they could be replaced with allocatables?
There was a problem hiding this comment.
It's probably the arrays, but it would need figuring out. Keep in mind windows has a 1MB default, while Linux usually has 8MB. This brings it to 8MB.
There was a problem hiding this comment.
The test cases explicitly state that they may require quite absurd stack sizes. 48MB is huge. Not sure why you would ever need that. And clearly was an overestimation.
There was a problem hiding this comment.
This is really weird. Not sure I understand the reason for why it was done like this. The first thing that is done is allocating 1MB of random data on the stack. https://github.com/fortran-lang/stdlib/blob/master/test%2Fhashmaps%2Ftest_open_maps.f90#L32
While the comment was saying 48MB that was for values of rand_power above 18. rand_size is 2**rand_power. rand_object is an integer array of rand_size elements. So this immediately overflows the stack on windows.
There was a problem hiding this comment.
I'm not familiar with the test if indeed, as an unit test does it really require such a large array? moving the array to the heap using an allocatable seems like a good solution to avoid the stack overflow, so this specific hack to increase the stack size would be not required.
This PR addresses #979 and #971 partially.
For intel compiler some dependencies with mvsc need to be managed.
Another problem is the stack memory size limit that needs to be increased for Windows.
cc @R-Goc @zoziha @jvdp1 @perazz