Enable feature stubprecode dynamic helpers again #115746
Conversation
… stream of assembly data instead of the current approach.
…e DAC. cDAC isn't done yet, but it should be simpler than the confusion that is today's implementation.
|
Tagging subscribers to this area: @mangod9 |
…STUBPRECODE_DYNAMIC_HELPERS_again
…STUBPRECODE_DYNAMIC_HELPERS_again
|
/azp list |
|
/azp run runtime-diagnostics |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for dynamic helper stub precodes (version 3) by switching to a full-byte-pattern comparison approach and integrates it into both the managed contracts and native runtime metadata.
- Introduces
PrecodeStubs_3_ImplwithReadBytesAndComparefor stub/fixup detection and updates the factory to return version 3. - Extends
PrecodeMachineDescriptor(native) with full stub/fixup byte arrays and ignore masks, and initializes them inCDacPlatformMetadata. - Updates allocation and detection in
precode.h/.cpp, VM startup, DAC tables, and ARM64 thunk templates to use the new patterns.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| PrecodeStubs_3.cs | New v3 contract API using full-byte comparisons |
| PrecodeStubs_2.cs, PrecodeStubs_1.cs | Ensure v2/v1 chaining still works |
| PrecodeStubsFactory.cs | Register version 3 in factory switch |
| readytoruninfo.cpp, loaderallocator.hpp | Switch dynamic helper allocation to AllocStub() |
| precode.h, precode.cpp | Remove magic offsets, add Is*ByASM_DAC, use full-byte compare |
| cdacplatformmetadata.{hpp,cpp}, dacvars.h, datadescriptor.h | Add new DAC fields & initialization for byte-pattern data |
| arm64 thunktemplates.* | Insert memory-barrier (dmb ishld) for stubs |
| contracts.jsonc | Bump PrecodeStubs contract version to 3 |
| dactable.cpp, enummem.cpp | Include new platform metadata header |
| clrfeatures.cmake | Enable FEATURE_STUBPRECODE_DYNAMIC_HELPERS on AMD64/ARM64 |
| PrecodeStubs.md | Document v3 byte-pattern approach |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
/azp run runtime-diagnostics |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Any numbers for the worst-case perf regressions that we expect to get from this change? |
Thank you very much and sorry for late reply, based on the latest commit: Release-CoreRoot-SPC-PE32+-pri1: |
|
@max-charlamb Could you please review the new Precode data contract and the cDac related changes? |
max-charlamb
force-pushed
the
enable_FEATURE_STUBPRECODE_DYNAMIC_HELPERS_again
branch
from
dc2f322 to
71b3876
Compare
...ged/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/PrecodeMachineDescriptor.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
#117382 passed runtime-diagnostics
lgtm from a cDAC perspective
This comment was marked as outdated.
This comment was marked as outdated.
RISC-V Release-CLR-QEMU: 9080 / 9110 (99.67%)report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-VF2: 508987 / 510675 (99.67%)report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
davidwrighton
dismissed
sirntar’s stale review
@sirntar isn't responding, and I believe I've addressed his feedback.
RISC-V Release-CLR-QEMU: 9080 / 9110 (99.67%)report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-CLR-VF2: 9081 / 9111 (99.67%)report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-QEMU: 275736 / 276816 (99.61%)report.xml, report.md, failures.xml, testclr_details.tar.zst RISC-V Release-FX-VF2: 307712 / 309349 (99.47%)report.xml, report.md, failures.xml, testclr_details.tar.zst Build information and commandsGIT: |
There was a problem hiding this comment.
LGTM
@davidwrighton sorry for delay, I was waiting for the tests to finish to see if there would be any more problems
0689a0b is being scheduled for building and testingGIT: |
|
@EgorBot -arm using System;
using System.Runtime.InteropServices;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace PInvokeBenchmark
{
public class PInvokeBenchmarks
{
// Define a delegate type for our managed function
private delegate int ManagedDelegate(bool value);
// Define a matching function pointer type for unmanaged calls
private unsafe delegate int UnmanagedDelegate(bool value);
// The managed implementation
private static int ManagedImplementation(bool value)
{
return value ? 42 : 0;
}
// Our instance of the delegate
private readonly ManagedDelegate _managedDelegate;
// Marshaled function pointer
private readonly unsafe delegate* unmanaged<bool, int> _nativeFunctionPointer;
// Delegate wrapping the marshaled function pointer
private readonly unsafe delegate* unmanaged<bool, int> _unmanagedDelegate;
// P/Invoke declaration for the native method
[DllImport(@"C:\temp\pinvokeBench\NativeDll\NativeDll.dll", CallingConvention = CallingConvention.Cdecl)]
private static extern int Method(bool value);
public unsafe PInvokeBenchmarks()
{
// Create the managed delegate
_managedDelegate = ManagedImplementation;
// Marshal it to a native function pointer
_nativeFunctionPointer = (delegate* unmanaged<bool, int>)Marshal.GetFunctionPointerForDelegate(_managedDelegate);
}
[Benchmark]
public unsafe int MarshaledDelegateCall()
{
return _nativeFunctionPointer(true);
}
}
} |
9 participants
Use the barrier approach to make this safe. Adding an address dependency got into lots of details around possibly needing an entire new stub type, and several paths through this code already depend on the concept of having a barrier here. To support modifying the stubs without inserting lots of magic number, rework how stubs are identified to just do a complete compare of the entire assembly stub in a way which will be easily to generalize to the cDAC.
In addition, make changes to make the FixupPrecode path safe from the same memory ordering concerns. Since the FixupPrecode always goes through a specified entrypoint, we can slide the barrier into only the slow path as long as we ensure that the Target field written with a VolatileStore after the
MethodDescandPrecodeFixupThunkfields are set. To ensure that the barrier on the writer side has happened before with hit the barrier on the reader, we ensure that the Target field is initialized into an infinite loop before the code associated with the FixupPrecode is mapped into the process.Fixes #113810
Also, this is now only enabled for scenarios where it is required. (Mac Catalyst and iOS).