Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reporting byrefs to RVA static fields of collectible assemblies #40346

Merged

Conversation

davidwrighton
Copy link
Member

Add support for reporting byrefs to RVA static fields of collectible assemblies

  • Keep track of all RVA static field locations

    • For assemblies loaded from PE files, use a range that is the entire PE range
    • For assemblies dynamically created, use piecemeal ranges for each individual RVA static field
  • Report byref references via the GcReportLoaderAllocator mechanism in PromoteCarefully

  • Add a test to cover this scenario

fixes #13348

…assemblies

- Keep track of all RVA static field locations
  - For assemblies loaded from PE files, use a range that is the entire PE range
  - For assemblies dynamically created, use piecemeal ranges for each individual RVA static field
- Report byref references via the GcReportLoaderAllocator mechanism in PromoteCarefully

- Add a test to cover this scenario
@davidwrighton davidwrighton requested a review from janvorli August 4, 2020 23:20
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2020
@davidwrighton davidwrighton added area-VM-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 4, 2020
@MichalStrehovsky
Copy link
Member

Do we have a similar problem when the RVA static is in the TLS range?

I assume this is the sort of stuff C++/CLI would produce (...or IL with .data tls).

@davidwrighton
Copy link
Member Author

C++/CLI is incompatible with collectible assemblies in many ways, so we can't support it properly in any case.
For IL with .data tls, this will not fix the issue, but there is little evidence that there are customers which will use the feature.

For regular thread static fields, you are right, that is worth a test, and I've tweaked the test to include that scenario. However, the way it works in CoreCLR today is ThreadStatic fields use heap objects in any case, so the test passes just fine.

@davidwrighton davidwrighton merged commit 69b91f9 into dotnet:master Aug 5, 2020
@xiangzhai
Copy link
Contributor

:mips-interest

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
…assemblies (dotnet#40346)

- Keep track of all RVA static field locations
  - For assemblies loaded from PE files, use a range that is the entire PE range
  - For assemblies dynamically created, use piecemeal ranges for each individual RVA static field
- Report byref references via the GcReportLoaderAllocator mechanism in PromoteCarefully

- Add a test to cover this scenario, and thread statics
  - disable test on Mono, as it doesn't pass there yet
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@davidwrighton davidwrighton deleted the refPointerIntoCollectibleAssembly branch April 20, 2021 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw array data does not count as collectible AssemblyLoadContext root?
6 participants