Fix NativeAOT ThunksPool thunk data block size handling. #111149
Merged
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
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
This was referenced Jan 7, 2025
This was referenced Jan 7, 2025
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkotas
reviewed
Jan 9, 2025
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/ThunkPool.cs
Outdated
Show resolved
Hide resolved
jkotas
approved these changes
Jan 9, 2025
Updated version of dotnet#110732 fixing issues on ARM32 and other platforms where size diffrence between code and data thunk caused thunk data block size calculations to be to small. dotnet#88710 made a change in TunkPool.cs moving away from using a page size define to calling ThunkBlockSize to get to end of thunk data block where a common stub address get stored. This change is not equivalent on platforms where the thunk blocks are laid out in pair where a stub thunk blocks are followed by a data thunk block and all gets mapped from file. This is the schema used on Windows platforms. In that layout schema the ThunkBlockSize is 2 * page size meaning that the calculation getting to the end of the thunk data block will move to the end of next thunk stub that is RX memory and storing the common stub address at that location will trigger an AV. This works on iOS since it reports its ThunkBlockSize as one page but that is not totally correct since it uses 2 pages, just that they are allocated in the same way as FEATURE_RX_THUNKS, all thunk stubs blocks followed by all thunk data blocks. The reason why this works is because it only maps the thunk stubs from file, reporting a ThunkBlockSize that is inline with what gets map:ed from file, but then there is a special handling in PalAllocateThunksFromTemplate on iOS that virutal alloc template size * 2, mapping the first template size bytes from the file and the rest are kept as its thunk data blocks. This commit calculates the page size of code/data block based on max of code/data thunk size * number of thunks per block and since this is guaranteed to fit into one block and that the block size needs to be a power of 2, the correct full block size used in arch specific implementation when laying out the stub and data blocks can be calculated directly in managed code. Review feedback. Switch to ThunkDataBlockSizeMask in one place. Fix build error. Include pointer size slot in data block size calculation. Calculate page size using NumThunksPerBlock.
lateralusX
force-pushed
the
lateralusX/fix-thunkpool-av-2
branch
from
8b85ca2 to
66e68e5
Compare
3 tasks
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jkotas Outerloop cancelations seems to be inline with other PR's. Do we believe we have enough proof that this change is good to go? |
I'm not concerned about the sizeopt/speedopt variations. The plain windows leg succeeded so this looks good! Thanks! |
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.
Updated version of #110732 fixing issues on ARM32 and other platforms where size diffrence between code and data thunk caused thunk data block size calculations to be to small.
#88710 made a change in TunkPool.cs
moving away from using a page size define to calling ThunkBlockSize to get to end of thunk data block where a common stub address get stored.
This change is not equivalent on platforms where the thunk blocks are laid out in pair where a stub thunk blocks are followed by a data thunk block and all gets mapped from file. This is the schema used on Windows platforms.
In that layout schema the ThunkBlockSize is 2 * page size meaning that the calculation getting to the end of the thunk data block will move to the end of next thunk stub that is RX memory and storing the common stub address at that location will trigger an AV.
This works on iOS since it reports its ThunkBlockSize as one page but that is not totally correct since it uses 2 pages, just that they are allocated in the same way as FEATURE_RX_THUNKS, all thunk stubs blocks followed by all thunk data blocks. The reason why this works is because it only maps the thunk stubs from file, reporting a ThunkBlockSize that is inline with what gets map:ed from file, but then there is a special handling in PalAllocateThunksFromTemplate on iOS that virutal alloc template size * 2, mapping the first template size bytes from the file and the rest are kept as its thunk data blocks.
This commit calculates the page size of code/data block based on max of code/data thunk size * number of thunks per block and since this is guaranteed to fit into one block and that the block size needs to be a power of 2, the correct full block size used in arch specific implementation when laying out the stub and data blocks can be calculated directly in managed code.