Extract out minimal concerns from PEImageLayout for R2R images #120777
Conversation
jkoritzinsky
added
area-ReadyToRun-coreclr
blocked
…determining if a given instruction is in an address range.
…from a R2R image without requiring the PE format
jkoritzinsky
force-pushed
the
r2r-native-layout-non-pe
branch
from
7d54255 to
f654ce0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a lightweight ReadyToRunLoadedImage abstraction to decouple ReadyToRun (R2R) processing from PEImageLayout, easing future support for non-PE (e.g. Mach-O) envelopes.
- Replaces usages of PEImageLayout/PEDecoder for R2R access with ReadyToRunLoadedImage.
- Adds ReadyToRunLoadedImage class with basic image base/size access and cleanup semantics.
- Updates multiple subsystems (R2R info, native image loading, PGO, fixups, DAC) to use the new abstraction.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/readytoruninfo.h | Adds ReadyToRunLoadedImage class and replaces PEImageLayout pointers with the new type. |
| src/coreclr/vm/readytoruninfo.cpp | Constructs ReadyToRunLoadedImage during ReadyToRunInfo initialization. |
| src/coreclr/vm/prestub.cpp | Switches fixup helpers to use ReadyToRunLoadedImage. |
| src/coreclr/vm/pgo.h | Updates PGO interfaces to use ReadyToRunLoadedImage. |
| src/coreclr/vm/pgo.cpp | Adapts instrumentation reader to new image type. |
| src/coreclr/vm/nativeimage.h | Replaces PEImageLayout member with ReadyToRunLoadedImage. |
| src/coreclr/vm/nativeimage.cpp | Wraps PEImageLayout in ReadyToRunLoadedImage with cleanup lambda. |
| src/coreclr/vm/frames.cpp | Uses ReadyToRunLoadedImage for GC ref map lookup. |
| src/coreclr/vm/codeman.cpp | Updates EH enumeration to use new image abstraction. |
| src/coreclr/vm/ceeload.inl | Template fixup function now takes ReadyToRunLoadedImage. |
| src/coreclr/vm/ceeload.h | Module accessors updated to return ReadyToRunLoadedImage. |
| src/coreclr/vm/ceeload.cpp | Module methods updated to use ReadyToRunLoadedImage. |
| src/coreclr/debug/daccess/request.cpp | DAC tiered version query updated to use ReadyToRunLoadedImage and GetVirtualSize. |
|
Is the plan to nest a complete PE image inside the MachO envelope or the PE part might go away at some point? Not that it should block this PR at all, but just wanted to give a heads up that its very likely there are lurking assumptions about the PE file format in the debugging stack. For example we have: If we get to the point where that base address doesn't point to PE format then debugger changes will be needed. |
There was a problem hiding this comment.
This seems fine to me from the diagnostics angle. In the future if we get to the point that PE headers go away or the GetBaseAddress API isn't pointing to it then we've got some debugger impact to work through.
|
The plan is to not have a fake PE header in the MachO format and only have the R2R data structures. Given that GetBaseAddress returns 0 for native images, I would expect the MachO files to go down the same route for the debugger APIs. |
|
/ba-g Android timeout |
3 participants
Depends on #120758
Will make it much easier to implement support for a Mach-O envelope of R2R data.
Contributes to #120065