Skip to content

Reapply "Create a single copy of stub templates #115749

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

Merged
merged 3 commits into from
May 20, 2025

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented May 19, 2025

Revert PR #115665 which reverted #114462

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@davidwrighton davidwrighton marked this pull request as ready for review May 19, 2025 23:25
@Copilot Copilot AI review requested due to automatic review settings May 19, 2025 23:25
@davidwrighton davidwrighton requested review from thaystg and hoyosjs May 19, 2025 23:25
Copy link
Contributor

@Copilot 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 reverts previous changes that affected the creation of a single copy of stub templates and disables the feature on Apple platforms pending resolution of dotnet/diagnostics#5487.

  • Reintroduces changes to use standard integer types (uint8_t and size_t) in function signatures.
  • Integrates a new configuration structure (InterleavedLoaderHeapConfig) for loader heap initialization and code page generation.
  • Disables the template mapping feature on Apple platforms and adds related conditional logic.

Reviewed Changes

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

Show a summary per file
File Description
src/coreclr/vm/precode.h / precode.cpp Updated function signatures to use uint8_t* and size_t for improved clarity and consistency.
src/coreclr/vm/loaderallocator.cpp Replacing function pointer parameters with references to InterleavedLoaderHeapConfig for stub code page generation.
src/coreclr/utilcode/interleavedloaderheap.cpp Updated allocation paths to support template-based thunk allocation and proper configuration initialization.
src/coreclr/inc/loaderheap.h Added and updated the configuration structure and its usage in loader heap initialization.
Other files Adjustments in assembly and executable allocator files to integrate template-based thunk allocation.
Comments suppressed due to low confidence (3)

src/coreclr/utilcode/interleavedloaderheap.cpp:542

  • The integration of the new InterleavedLoaderHeapConfig and its initialization via CreateTemplate appears correct. Please ensure that the template-based allocation paths are covered by adequate tests to validate the behavior under both standard and template-enabled environments.
void InitializeLoaderHeapConfig(InterleavedLoaderHeapConfig *pConfig, size_t stubSize, void* templateInImage, void (*codePageGenerator)(uint8_t* pageBase, uint8_t* pageBaseRX, size_t size))

src/coreclr/vm/precode.h:228

  • Changing function signatures from BYTE* and SIZE_T to uint8_t* and size_t improves type clarity and portability. Please ensure that all related pointer arithmetic and casting remain consistent across the codebase.
static void GenerateCodePage(uint8_t* pageBase, uint8_t* pageBaseRX, size_t size);

src/coreclr/vm/loaderallocator.cpp:1211

  • Replacing the original code page generator function pointer with a reference to the global configuration ensures a centralized management approach. Verify that all caller sites of the loader heap allocation have been updated to work with the new configuration structure.
&s_stubPrecodeHeapConfig

@thaystg
Copy link
Member

thaystg commented May 19, 2025

I think we need to merge and release dbgshim, then bump it everywhere and then merge here. What do you think @tommcdon @davidwrighton ?

@thaystg
Copy link
Member

thaystg commented May 19, 2025

Ah okay, we should enable on Mac after doing what I asked :)

Copy link
Member

@tommcdon tommcdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment change for your review

Co-authored-by: Tom McDonald <tommcdon@microsoft.com>
@davidwrighton
Copy link
Member Author

/ba-g The build break is unrelated and has already been fixed by #115767

@davidwrighton davidwrighton merged commit 35514ca into dotnet:main May 20, 2025
93 of 95 checks passed
SimaTian pushed a commit that referenced this pull request May 27, 2025
* Reapply "Create a single copy of stub templates (#114462)" (#115665)

This reverts commit f7fc178.

* Disable feature on Apple platforms for now

Co-authored-by: Tom McDonald <tommcdon@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants