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 |
| #endif | ||
| } | ||
|
|
||
| void CDacPlatformMetadata::InitPrecodes() | ||
| { | ||
| PrecodeMachineDescriptor::Init(&(&g_cdacPlatformMetadata)->precode); |
There was a problem hiding this comment.
The expression &(&g_cdacPlatformMetadata)->precode is overly verbose. It can be simplified to &g_cdacPlatformMetadata.precode for clarity.
| PrecodeMachineDescriptor::Init(&(&g_cdacPlatformMetadata)->precode); | |
| PrecodeMachineDescriptor::Init(&g_cdacPlatformMetadata.precode); |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/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? |
| ldr x10, DATA_SLOT(StubPrecode, Target) | ||
| ldr x12, DATA_SLOT(StubPrecode, SecretParam) | ||
| br x10 | ||
| brk 0xf000 // Stubs need to be 24-byte in size to allow for the data to be 3 pointers | ||
| brk 0xf000 // Stubs need to be 24-byte in size to allow for the data to be 3 pointers | ||
| LEAF_END_MARKED StubPrecodeCode\STUB_PAGE_SIZE | ||
|
|
||
| LEAF_ENTRY FixupPrecodeCode\STUB_PAGE_SIZE |
There was a problem hiding this comment.
Why can we skip the barrier for FixupPrecode?
| @@ -120,9 +120,12 @@ LEAF_END_MARKED CallCountingStubCodeTemplate, _TEXT | |||
| .irp STUB_PAGE_SIZE, 16384, 32768, 65536 | |||
There was a problem hiding this comment.
Does FEATURE_MAP_THUNKS_FROM_IMAGE above need the same change?
There was a problem hiding this comment.
Sigh, I missed that part of the merge. Yes.
| @@ -120,9 +120,12 @@ LEAF_END_MARKED CallCountingStubCodeTemplate, _TEXT | |||
| .irp STUB_PAGE_SIZE, 16384, 32768, 65536 | |||
|
|
|||
| LEAF_ENTRY StubPrecodeCode\STUB_PAGE_SIZE | |||
| dmb ishld | |||
There was a problem hiding this comment.
Do we need a counter-part barrier on the writer sides (e.g. in DynamicHelperFixup)?
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.
Fixes #113810