-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Place async resumption info in read-write section outside Windows for AOT #123433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Rework `allocMem` to support allocating an arbitrary number of data chunks. The motivation is to allow ilc to put async resumption info chunks into sections that support relocations to .text, but additionally will allow for sharing read only data between multiple functions.
This reverts commit 056f51b.
|
cc @dotnet/jit-contrib PTAL @MichalStrehovsky (NAOT parts) @EgorBo (JIT parts) |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| if ((dataChunks[i].flags & CORJIT_ALLOCMEM_HAS_POINTERS_TO_CODE) != 0) | ||
| { | ||
| // These are always passed to the EE as separate chunks since their relocations need special treatment | ||
| dataChunks[i].block = chunks.BottomRef(curDataChunk).block; | ||
| dataChunks[i].blockRW = chunks.BottomRef(curDataChunk).blockRW; | ||
| curDataChunk++; | ||
| } | ||
| else | ||
| { | ||
| dataChunks[i].block = codeChunk.block + (size_t)dataChunks[i].block; | ||
| dataChunks[i].blockRW = codeChunk.blockRW + (size_t)dataChunks[i].blockRW; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: it seems questionable whether we need this "put into .text" behavior at all. And if we do (due to distance constraints), then presumably we could hit problems for async resumption info too.
If we end up with the optimization to fold common read only data then we will probably want to disable this so that it also kicks in for arm64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes issue #121871 by placing async resumption data structures containing relocations to code sections into read-write memory sections on ELF/MachO platforms (non-Windows). The PE file format used on Windows supports relocations from read-only sections to code sections, but ELF and MachO do not, causing linking errors. The fix introduces a new MethodReadWriteDataNode class and teaches both the JIT and AOT compilers to route data with the CORJIT_ALLOCMEM_HAS_POINTERS_TO_CODE flag to read-write sections when targeting non-Windows platforms.
Changes:
- Introduces
MethodReadWriteDataNodeto hold data with relocations to code in read-write sections - Modifies JIT and AOT memory allocation logic to conditionally place async resumption info in RW sections on ELF/MachO
- Removes platform-specific test restrictions that were workarounds for this issue
- Reverts temporary workaround in
MethodReadOnlyDataNodethat placed all data in RW sections on non-Windows
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/nativeaot/SmokeTests/UnitTests/UnitTests.csproj | Removes x64-only condition for runtime-async feature, enabling it for all architectures |
| src/tests/async/Directory.Build.targets | Removes arm64 nativeaot build exclusion workaround |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Adds MethodReadWriteDataNode to ReadyToRun compiler project |
| src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj | Adds MethodReadWriteDataNode to AOT compiler project |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Adds RW data handling parallel to existing RO data handling, routes chunks with pointers to code based on platform |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/MethodReadWriteDataNode.cs | New node class for read-write method data, mirrors MethodReadOnlyDataNode structure |
| src/coreclr/tools/Common/Compiler/DependencyAnalysis/MethodReadOnlyDataNode.cs | Reverts workaround that placed all data in RW sections on non-Windows platforms |
| src/coreclr/jit/ee_il_dll.cpp | Modifies JIT allocation to pass chunks with CORJIT_ALLOCMEM_HAS_POINTERS_TO_CODE as separate chunks on ARM64/LoongArch64/RISCV64 |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ELF/MachO do not support relocations from read-only sections into the .text section. We generate such relocations when creating async resumption info. Now that #123378 is in we can teach ilc/crossgen2 to place these pieces of data in a read-write section when necessary.
Fix #121871