Skip to content

[clr-interp] allow modification of the this pointer in functions which use it as the generics context#119338

Closed
davidwrighton wants to merge 93 commits intodotnet:mainfrom
davidwrighton:fix_mutate_generics_this_pointer
Closed

[clr-interp] allow modification of the this pointer in functions which use it as the generics context#119338
davidwrighton wants to merge 93 commits intodotnet:mainfrom
davidwrighton:fix_mutate_generics_this_pointer

Conversation

@davidwrighton
Copy link
Member

  • When the this pointer is used as the generics context, if some code modifies the this pointer, do not allow that modification to affect the generics behavior of the function
  • Implement this by making a shadow copy of the this pointer in this situation
  • To avoid doing this ALL of the time, I've added a scheme where we can restart the entire method compilation by setting a flag which describes the current set of retry rules.

…h use it as the generics context

- When the this pointer is used as the generics context, if some code modifies the this pointer, do not allow that modification to affect the generics behavior of the function
- Implement this by making a shadow copy of the this pointer in this situation
- To avoid doing this ALL of the time, I've added a scheme where we can restart the entire method compilation by setting a flag which describes the current set of retry rules.
Copilot AI review requested due to automatic review settings September 3, 2025 23:17
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 3, 2025
Copy link
Contributor

Copilot AI left a 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 implements a retry mechanism in the CLR interpreter to handle cases where the this pointer is modified in generic methods that use it as the generics context. The solution creates a shadow copy of the this pointer to prevent modifications from affecting generic behavior.

Key changes:

  • Added a retry compilation loop that restarts method compilation when specific conditions are detected
  • Introduced InterpreterRetryFlags to control retry behavior with a specific flag for this pointer mitigation
  • Modified variable creation and instruction emission to detect when the this pointer might be modified and enable the shadow copy mechanism

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/coreclr/interpreter/eeinterp.cpp Added retry compilation loop with flag-based restart mechanism
src/coreclr/interpreter/compiler.h Added InterpreterRetryFlags enum and updated constructor signature
src/coreclr/interpreter/compiler.cpp Implemented shadow copy logic, retry detection, and variable management for this pointer mitigation

alexey-zakharov and others added 6 commits September 3, 2025 17:05
…oading of assemblies cached by TypeDescriptor (dotnet#114619)

* Use separate hashtables for collectible types to support assembly unloadability for the TypeDescriptor class

* Addressed codereview feedback

* Added a test for custom provider and updated WeakHashTable to use ConditionalWeakTable to ensure values can be also colelcted

* Renamed ContextAwareHashtable to CollectibleKeyHashtable
…on the context param (dotnet#119336)

- Add support to the InterpreterCodeManager for the ability to understand the generic context of a given frame
- This is done by adding implementation for InterpreterCodeManager::GetInstance, InterpreterCodeManager::GetParamTypeArg and InterpreterCodeManager::GetParamContextType
- In addition, provide an  implementation of InterpreterJitManager::ResolveEHClause. The enhancement with this is that it will support doing shared generic catch of a generically dependent exception type. (The JIT implements this by translating these sorts of catches into filters, but that doesn't seem like a good approach for the interpreter. The existing function is noted as being sensitive to stack overflow issues, so I didn't update this for all scenarios)

As a bonus, this work also enables !clrstack -i -a to understand the actual accurate generic types of all of the locals.
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>
SingleAccretion and others added 14 commits September 4, 2025 10:24
…dotnet#118778)

Today physical promotion will create `FIELD_LIST` for returned struct values. However, arguments did not get the same treatment and had constraints motivated by old limitations of the backend. Since those limitations are gone now we can just indiscriminately produce `FIELD_LIST` in physical promotion.
Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 4 to 5.
- [Release notes](https://github.com/actions/setup-dotnet/releases)
- [Commits](actions/setup-dotnet@v4...v5)

---
updated-dependencies:
- dependency-name: actions/setup-dotnet
  dependency-version: '5'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Fix ProfileEnter when called on a foreign thread

When ProfileEnter is called on a foreign thread that the runtime has not
seen yet, which can happen for UnmanagedCallersOnly marked methods,
it crashes in the GCX_COOP_THREAD_EXISTS(GET_THREAD()); That was left in
by accident in my change I've made a long time ago and should not be
there.
This change removes that switch, we switch to cooperative mode later in
the ProfileEnter and setup the thread for runtime before that.

Close dotnet#115617

* Make ProfileLeave look the same
Porting feedback from dotnet#119195 to main.
* fix bug where waiting threads were not iterated
…otnet#119334)

Windows builds now enable /guard:cf. Also pick up a risc-v enhancement.
* Limit doublemapper maximum size

The double mapping currently sets the maximum size of the double mapped memory to 2TB.
However, that causes the runtime to fail to start if there is a rlimit for max file
size set to a smaller value.

This change fixed the problem by clipping the max double mapped memory size by that
rlimit and also uses much more conservative size even without the file size limit set.
It is now set to the total amount of physical memory by default and also clipped by
the virtual memory rlimit.

Close dotnet#117819

* In case of VM limit, use just 1/4 of the space

* Change physicalMemorySize to 64 bits

Even on 32 bit targets, more than 4GB memory may be available.
…cCatalyst (dotnet#119177)

* [clr-interp] Setup fully interpreted functional test on Apple mobile

* Run tests in runtime.yml pipeline

* Run arm64 architecture only

* Fix build

* Remove tvOS platform

* Test linker option

* Fix compiler flag

* Link interpreter into coreclr library

* Fix wasm build

* Test maccatalyst build

* Revery no-overriding-t-option flag

* Test maccatalyst build

* Revert no-overriding-t-option flag

* Test maccatalyst build

* Add SystemNative_Log and SystemNative_LogError declarations

* Remove tvos_arm64 platform

* Update APPLE_RUNTIME_IDENTIFIER to use placeholder

* Update maccatalyst build args

* Use checked configuration

* Remove iOS device build from runtime pipeline

* Fix interpreter build exports

* Fix failing build

* Fix R2R condition

* Test debug config

* Test checked build on MacCatalyst and simulator

* Test debug config

* Test checked configuration

* Fix comment

* Remove dependency on dn-containers

* Define PAL_JitWriteProtect macro for platforms that don't allow JIT write protection

* Update install_clr destination to sharedFramework

* Fix build
* Backflow from https://github.com/dotnet/dotnet / 3e92251 build 281912

[[ commit created by automation ]]

* Update dependencies from https://github.com/dotnet/dotnet build 281912
Updated Dependencies:
Microsoft.CodeAnalysis, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.CSharp, Microsoft.Net.Compilers.Toolset (Version 5.0.0-2.25420.121 -> 5.0.0-2.25453.110)
Microsoft.CodeAnalysis.NetAnalyzers, Microsoft.DotNet.ApiCompat.Task, Microsoft.NET.Workload.Emscripten.Current.Manifest-10.0.100.Transport (Version 10.0.100-rc.1.25420.121 -> 10.0.100-rc.1.25453.110)
Microsoft.DotNet.Arcade.Sdk, Microsoft.DotNet.Build.Tasks.Archives, Microsoft.DotNet.Build.Tasks.Feed, Microsoft.DotNet.Build.Tasks.Installers, Microsoft.DotNet.Build.Tasks.Packaging, Microsoft.DotNet.Build.Tasks.TargetFramework, Microsoft.DotNet.Build.Tasks.Templating, Microsoft.DotNet.Build.Tasks.Workloads, Microsoft.DotNet.CodeAnalysis, Microsoft.DotNet.GenAPI, Microsoft.DotNet.GenFacades, Microsoft.DotNet.Helix.Sdk, Microsoft.DotNet.PackageTesting, Microsoft.DotNet.RemoteExecutor, Microsoft.DotNet.SharedFramework.Sdk, Microsoft.DotNet.XliffTasks, Microsoft.DotNet.XUnitExtensions (Version 10.0.0-beta.25420.121 -> 11.0.0-beta.25453.110)
Microsoft.DotNet.Cecil (Version 0.11.5-alpha.25420.121 -> 0.11.5-alpha.25453.110)
Microsoft.DotNet.XUnitAssert, Microsoft.DotNet.XUnitConsoleRunner (Version 2.9.3-beta.25420.121 -> 2.9.3-beta.25453.110)
Microsoft.NET.Sdk.IL, Microsoft.NETCore.App.Ref, Microsoft.NETCore.ILAsm, runtime.native.System.IO.Ports, System.Reflection.Metadata, System.Reflection.MetadataLoadContext, System.Text.Json (Version 10.0.0-rc.1.25420.121 -> 10.0.0-rc.1.25453.110)
NuGet.Frameworks, NuGet.Packaging, NuGet.ProjectModel, NuGet.Versioning (Version 7.0.0-preview.1.42121 -> 7.0.0-preview.1.45410)
System.CommandLine (Version 2.0.0-rc.1.25420.121 -> 2.0.0-rc.1.25453.110)

* Remove duplicate definitions from emitriscv64

* Make tests clear out DOTNET_ROOT_<arch> when clearing DOTNET_ROOT

(cherry picked from commit 503100d)
(cherry picked from commit d201857)

* Fix environment variable casing

(cherry picked from commit db0d37a)
(cherry picked from commit d3ff91a)

---------

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>
kg and others added 6 commits September 10, 2025 07:47
* Re-enable some disabled crossgen2 tests that now pass
* Broaden exclusion for DevDiv_255294 since it is not an ARM specific failure
…119544)

This prints "ERROR: The process "corerun.exe" not found" under normal circumstances that makes it look like an actual test failure.

Fixes dotnet#119525
@davidwrighton
Copy link
Member Author

Ugh, something went wrong with a merge locally.... this needs to be cleaned up, as it shouldn't be doing this much in the change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-Interpreter-coreclr linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.