[cDAC] Handle targets without ReJIT in method versionability - #135054
radekdoulik wants to merge 3 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
lewing
left a comment
lewing
left a comment
There was a problem hiding this comment.
This matches native: MethodDesc::IsEligibleForReJIT returns false without FEATURE_REJIT, and the descriptor only advertises ReJIT under PROFILING_SUPPORTED. Treating ContractMissingException as "feature absent" while rethrowing unrecognized versions and read failures is the right split. Gating on the contract rather than on architecture fits here, because the driver is the profiler build setting, not WASM itself. I ran IsVersionable tests locally at 7679c6f: 28/28 pass.
One related gap: CoreCLRContracts.ValidateForDataAccess still calls Validate<IReJIT>(registry) unconditionally, so a target built without profiling support still fails validation before IsVersionable is reached. #135044 had to gate Validate<IDebugger> on WASM for the same reason. Either gate it on the contract being advertised or note it as a follow-up. The SOSDacImpl callers of Contracts.ReJIT have the same assumption, but they aren't reachable on WASM today.
Note
This review was generated with assistance from GitHub Copilot.
| if (!_target.Contracts.TryGetContract(out IReJIT reJit, out System.Exception? failure)) | ||
| { | ||
| if (failure is ContractMissingException) | ||
| return false; | ||
| throw failure; | ||
| } |
There was a problem hiding this comment.
| if (!_target.Contracts.TryGetContract(out IReJIT reJit, out System.Exception? failure)) | |
| { | |
| if (failure is ContractMissingException) | |
| return false; | |
| throw failure; | |
| } | |
| if (!_target.Contracts.TryGetContract(out IReJIT reJit) | |
| { | |
| return false; | |
| } |
Use overload that takes care of throwing the exceptions?
Make the one-output TryGetContract overload return false only for missing contracts, keeping the two-output overload available for inspecting failures. Use it in IsVersionable and cover unsupported versions and creator failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Treat ReJIT as optional only when it is not advertised. Keep version errors and required-contract checks intact, and validate availability without instantiating ReJIT. Cover absence and unsupported versions across the four mock architectures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| return TryGetContract(out contract, out _); | ||
| if (!TryGetContract(out contract, out System.Exception? failureException)) | ||
| { | ||
| if (failureException is ContractMissingException) |
There was a problem hiding this comment.
I would expect TryGetContract(out contract, out System.Exception? failureException) to always return false when the requested contract is missing. Is that not the case? If there is a bug, should it be fixed inside TryGetContract(out contract, out System.Exception? failureException) implementation instead?
This branch has not been deployed
4 participants
Handle a missing ReJIT contract in
RuntimeTypeSystem.IsVersionablefor targetsbuilt without profiling support. Non-tiered methods return
false; the tieringfast path is unchanged. Invalid advertised contracts and target-read errors
still propagate.
Adds four-architecture tests for contract availability, tiering, versioning
support and error propagation, and updates the contract specification.
Validation: managed cDAC build and 3,260 unit tests passed; generated contract
docs are up to date. The missing-ReJIT cases fail without the fix.
NativeAOT publishing and live-browser tests were not rerun for this patch.
Note
Prepared with GitHub Copilot.