-
Notifications
You must be signed in to change notification settings - Fork 5.3k
ILCompiler.Reflection.ReadyToRun encapsulate image state #117446
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
Conversation
bcdb320 to
cf1c138
Compare
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 replaces raw byte[] access with a NativeReader stream-based reader across the ReadyToRun reflection and dumping tools, and updates project targets to use $(NetCoreAppMinimum) instead of netstandard2.0. Key changes include:
- Introduction of
NativeReaderfor all binary reads and replacement of staticNativeReader.ReadXcalls. - Initialization of
ImageReaderinReadyToRunReaderand propagation through all parsing classes. - Update of project files (
.csprojand.pkgproj) to use$(NetCoreAppMinimum)target framework.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TextDumper.cs | Swapped raw byte[] calls to ImageReader calls. |
| UnwindInfo.cs (x86, Arm, Arm64, Amd64, LoongArch64, RiscV64) | Replaced constructor and read methods to use NativeReader. |
| ReadyToRunReader.cs | Introduced ImageReader property and updated initializers. |
| ReadyToRunHeader.cs | Updated header parsing to use NativeReader, but contains an incorrect copy/reset sequence. |
| NativeReader.cs | Defined NativeReader class with instance methods, but class declaration syntax is invalid. |
| NativeHashtable.cs / NativeParser.cs / NativeArray.cs / NibbleReader.cs | Converted all binary reads to instance-based NativeReader. |
| csproj & pkgproj | Changed target frameworks from hardcoded netstandard2.0 to $(NetCoreAppMinimum). |
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/NativeReader.cs:13
- The class declaration syntax is invalid—constructor parameters belong on a constructor, not on the class itself. Change this line to
public class NativeReaderand add a matching constructor signature inside the class body:public NativeReader(Stream backingStream, bool littleEndian = true) { ... }.
public class NativeReader(Stream backingStream, bool littleEndian = true)
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunHeader.cs:135
- This call resets
curOffsetand overwrites thesignaturebuffer from an empty int array, discarding the bytes just read. Remove thecurOffset = startOffset;andArray.Copy(...)lines—use the data populated byReadSpanAtdirectly.
Array.Copy(Array.Empty<int>(), curOffset, signature, 0, sizeof(uint) - 1);
|
@dotnet/samsung Could you please take a look? These changes may be related to riscv64. |
bad6c75 is being scheduled for building and testingGIT: |
RISC-V Release-CLR-QEMU: 9082 / 9112 (99.67%)report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
0b54cd6 is being scheduled for building and testingGIT: |
Using
NativeReaderinstead of a rawbyte[]will allow cDAC to utilize existing R2R parsing tools in this package. This change also adds some support for reading both LE/BE images. However this isn't fully implemented.Modifies package to target netcore rather than netstandard.