Skip to content

Remove getTypeForBoxOnStack from jit interface #114754

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 4 commits into from
Apr 19, 2025

Conversation

AndyAyersMS
Copy link
Member

Along with the StackAllocatedBox type. The JIT can now represent on-stack boxes directly via class layouts.

Closes #114204

Along with the `StackAllocatedBox` type. The JIT can now represent on-stack
boxes directly via class layouts.

Closes dotnet#114204
@Copilot Copilot AI review requested due to automatic review settings April 16, 2025 21:48
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 16, 2025
@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

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.

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: Language not supported
Comments suppressed due to low confidence (1)

src/coreclr/inc/corinfo.h:2481

  • Confirm that all implementations of the ICorStaticInfo interface are updated to remove getTypeForBoxOnStack, ensuring interface consistency following its deletion.
virtual CORINFO_CLASS_HANDLE getTypeForBoxOnStack(CORINFO_CLASS_HANDLE cls) = 0;

@@ -2623,7 +2608,7 @@ private static uint _getJitFlags(IntPtr thisHandle, IntPtr* ppException, CORJIT_

private static IntPtr GetUnmanagedCallbacks()
{
void** callbacks = (void**)Marshal.AllocCoTaskMem(sizeof(IntPtr) * 177);
void** callbacks = (void**)Marshal.AllocCoTaskMem(sizeof(IntPtr) * 176);
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

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

Ensure that the updated callback array size (changed from 177 to 176) accurately reflects the removal of the getTypeForBoxOnStack callback and that all subsequent callback indices have been correctly adjusted.

Copilot uses AI. Check for mistakes.

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Apr 17, 2025

I am seeing failures in System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandlerTest.GetAsync_ManyDifferentResponseHeaders_ParsedCorrectly that were not there before the recent merge: console log

Wondering if #114759 might be related?

@MihaZupan FYI

Same test is failing in #114785

@MihaZupan
Copy link
Member

MihaZupan commented Apr 17, 2025

System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandlerTest.GetAsync_ManyDifferentResponseHeaders_ParsedCorrectly(newline: "\r\n", fold: "\r\n    ", dribble: True) [FAIL]
      Assert.Equal() Failure: Strings differ
                 ↓ (pos 0)
      Expected: "fname.ext"
      Actual:   ""fname.ext""

That's #114771, specifically the tests against .NET Framework, cc: @ManickaP.
I do see the failure on that PR (Build windows-x86 Release Libraries_NET481 failed), but build analysis was green 😕

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Apr 17, 2025

System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandlerTest.GetAsync_ManyDifferentResponseHeaders_ParsedCorrectly(newline: "\r\n", fold: "\r\n    ", dribble: True) [FAIL]
      Assert.Equal() Failure: Strings differ
                 ↓ (pos 0)
      Expected: "fname.ext"
      Actual:   ""fname.ext""

That's #114771, specifically the tests against .NET Framework, cc: @ManickaP. I do see the failure on that PR (Build windows-x86 Release Libraries_NET481 failed), but build analysis was green 😕

Opened #114790

@hez2010
Copy link
Contributor

hez2010 commented Apr 18, 2025

Does this support representing a boxed nullable value type as well?

@AndyAyersMS
Copy link
Member Author

Does this support representing a boxed nullable value type as well?

See #114716, though likely we never change types here as nullable box allocation happens via helpers currently, and once we do that we will need to call the getTypeForBox earlier.

@AndyAyersMS
Copy link
Member Author

Pretty sure none of these failures are related. There seems to be one test failure not covered by build analysis, it's a timeout in remote executor: #108210 (comment)

@AndyAyersMS AndyAyersMS merged commit 27d2321 into dotnet:main Apr 19, 2025
141 of 146 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: We can remove getTypeForBoxOnStack
4 participants