Fix crash in MAP_End when using Intel's new icx compiler and disable caching in setup-python GH action #2173
Conversation
This file contains 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
The new icx compiler is a bit smarter about checking for memory errors and a new one was discovered which causes OpenFAST to crap when calling MAP_End. The error is due to the OutputList nodes containing pointers to memory which has already been freed. This is due to the way the OutputList is constructed where nodes from other lists (containing pointers to string memory) were appended without allocating new memory. So when the original list was freed, these pointers became invalid, but then the OutputList tried to free them again, resulting in a double free error. To fix this issue, set_output_list was changed to allocate new memory for copies of these nodes.
andrew-platt
approved these changes
Apr 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request is ready to be merged.
Feature or improvement description
This PR provides two fixes:
MAP_End) caused by double freeing memory related to strings. This crash only occurred when using the new Intel icx compiler.setup-pythonGithub Action to disable caching of Python modules. This should fix the issue where regression tests fail becausesetup-pythoncan't findrequirements.txt.Related issue, if one exists
PR #2129 was a previous attempt to fix the issue with
setup-pythonand the regression tests which didn't succeed.Impacted areas of the software
Additional supporting information
The issue with MAP trying to free the memory twice looks like it has been around since the module was added. The problem stemmed from several nodes being added to the
OutputListby directly copying the data. These nodes contained pointers to strings which were allocated. These pointers were shared with nodes in other lists which were freed earlier inMAP_End. When theOutputListnodes were freed, the code tried to free the same pointers again, resulting in a crash. The issue was resolved by creating new nodes inOutputListwith a copy of the data instead of reusing the existing pointers. The amount of data copied is very small so this was a simpler solution than trying to track the pointer usage.This fix will need to be migrated to the other branches once approved.