Preserve IL for CoreCLR interpreter fallback on Apple mobile#130622
Preserve IL for CoreCLR interpreter fallback on Apple mobile#130622kotlarmilos with Copilot wants to merge 22 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
Co-authored-by: kotlarmilos <11523312+kotlarmilos@users.noreply.github.com>
Co-authored-by: kotlarmilos <11523312+kotlarmilos@users.noreply.github.com>
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This sounds like a bug in how we produce the R2R images for use with interpreter. The instruction set should be fixed in this case, and we should never reject the native code. If the actual machine supports more instruction set extensions that the fixed set, we should ignore them. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 19812166-4b05-40cc-9a81-8b80b909e10b
Relocate the helper from ReadyToRunCompilerContext into crossgen2 Program.cs as a private method, and propagate FEATURE_DYNAMIC_CODE_COMPILED to the crossgen2 project so the moved #if guard resolves identically for desktop. Reword the Vector<T> optimistic comment, drop the redundant comment in the non-runtime-codegen branch, and document GetFixedInstructionSetSupport. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 491f030c-d5ac-479b-b029-362afaa80813
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RResultChecker.cs:977
- EagerInstructionSetSupportHasNoUnsupportedEntries claims to validate the global eager baseline Check_InstructionSetSupport fixup, but it currently scans all import sections (including non-eager/method-level sections). This can produce false failures if any per-method Check_InstructionSetSupport signatures contain '-' entries.
Filter to eager import sections (ReadyToRunImportSectionFlags.Eager) to match the method's stated intent.
foreach (ReadyToRunImportSection section in reader.ImportSections)
{
if (section.Entries is null)
continue;
foreach (ReadyToRunImportSection.ImportSectionEntry entry in section.Entries)
{
if (entry.Signature is not null && entry.Signature.FixupKind == ReadyToRunFixupKind.Check_InstructionSetSupport)
signatures.Add(entry.Signature.ToString(options));
}
}
src/libraries/System.Reflection.Metadata/tests/Metadata/Decoding/SignatureDecoderTests.cs:266
- The stripped-IL detection uses raw opcode bytes (0x2A and 0xFE 0x24) without explaining what 0xFE 0x24 represents. Adding a brief comment (and a named constant for 0x2A) would make it clearer that this matches the crossgen2 IL-stripping sentinel (CopiedMethodILNode.s_minimalILBody).
var il = body.GetILBytes();
bool isStripped =
(il?.Length == 1 && il[0] == 0x2A) ||
(il?.Length == 2 && il[0] == 0xFE && il[1] == 0x24);
if (!isStripped)
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
| string instructionSetSupportString = ReadyToRunInstructionSetSupportSignature.ToInstructionSetSupportString(instructionSetSupport); | ||
| // code if the runtime environment doesn't support the specified instruction set. Targets that cannot generate | ||
| // code at runtime must not encode "must be absent" assertions, since a failing eager fixup is a fatal startup | ||
| // error with no JIT fallback (see ToInstructionSetSupportString). |
There was a problem hiding this comment.
since a failing eager fixup is a fatal startup error with no JIT fallback (see ToInstructionSetSupportString
If we have no JIT fallback, we want to make sure that everything both the AOT compiler and the runtime are configured to use the same fixed instruction set so that the instruction support eager fixup would never fail.
It is fine to delete the instruction support eager fixup as an optimization.
'Targets that cannot generate code at runtime must not encode "must be absent" assertions' reasoning looks suspect to me. It says "oh, this was failing at runtime so have stopped generating to make the failure go away". Is this just hiding a bug? Is this change still necessary to fix the issues that this PR is fixing?
There was a problem hiding this comment.
On Apple arm64 the baseline signature emits "must be absent" assertions for the opportunistic instruction sets that fall outside the fixed baseline. GetFixedInstructionSetSupport widens that absent set to the full complement of the fixed baseline, and the device hardware actually provides many of those sets, so the eager Check_InstructionSetSupport fixup fails at load and disables all ReadyToRun code. With IL stripped and no JIT there is no fallback, so startup crashes.
Crossgen2 already fixes the required set through Helpers.GetFixedInstructionSetSupport, and the device has all of it, so the required half already agrees and the check passes. The excluded set is the half that pinning cannot fix. It says the CPU must not have the optional features the code skipped. Apple hardware does have those features, so this check always fails on that hardware. A failed fixup is fatal without a JIT. That is why aligning the fixed set does not prevent the failure.
On a JIT target a mismatch rejects the baseline method and the JIT recompiles for the richer hardware. Without a JIT that path does not exist, so their only effect is to reject valid fixed-baseline code and crash.
I kept the "must be present" half as an invariant check, since those sets are guaranteed present on these targets. I can drop the whole eager fixup for no-JIT targets instead if you prefer that.
crossgen2 binaries are host-specific, so gating GetTargetAllowsRuntimeCodeGeneration on the host build's FEATURE_DYNAMIC_CODE_COMPILED define is wrong. Drop the define from crossgen2.props and the surrounding #if from Program.cs, leaving the decision purely on the target OS and architecture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d421224-6ce0-4d3a-92ac-263330249e7c
…yToRun The define is no longer consumed by any source in the project after the runtime-codegen decision moved to a target-based check in crossgen2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2d421224-6ce0-4d3a-92ac-263330249e7c
|
/azp run runtime-extra-platforms |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Apple mobile composite ReadyToRun publishing could strip IL later required by the CoreCLR interpreter when compiled code is rejected or unavailable in a custom load context.
Stripping policy
Coverage