Skip to content

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 24, 2025

I've hacked a stress mode for GDV delegate inlining and found an issue that we don't validate return types.

Might fix #117867

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 24, 2025
@dotnet-policy-service
Copy link
Contributor

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

@jakobbotsch
Copy link
Member

Might fix #117867

That seems unlikely, this check should never fail with dynamic PGO, only with potentially outdated static PGO.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2025

Might fix #117867

That seems unlikely, this check should never fail with dynamic PGO, only with potentially outdated static PGO.

Yeah, but I think even for Dynamic PGO we should be resilient there (I know there is a debug assert there, but who knows).

@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2025

Still, the issue might be the Static PGO? The one that we ship BCL with,.

@jakobbotsch
Copy link
Member

I've hacked a stress mode for GDV delegate inlining and found an issue that we don't validate return types.

How did this issue surface? The point of the check is just to ensure that the JIT does not blow up. If the signatures don't match then the guard will never pass at runtime, so I don't think it would ever result in bad codegen.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2025

I've hacked a stress mode for GDV delegate inlining and found an issue that we don't validate return types.

How did this issue surface? The point of the check is just to ensure that the JIT does not blow up. If the signatures don't match then the guard will never pass at runtime, so I don't think it would ever result in bad codegen.

Makes sense, but in my case it just threw a bunch of JIT asserts here and there

@EgorBo
Copy link
Member Author

EgorBo commented Jul 25, 2025

Does it look good? @jakobbotsch cc @dotnet/jit-contrib

@EgorBo EgorBo requested a review from jakobbotsch July 25, 2025 07:27
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 adds return type validation to the isCompatibleMethodGDV function in the JIT compiler to prevent incorrect delegate inlining optimizations. The change addresses a bug discovered through stress testing where Guarded Devirtualization (GDV) for delegate calls was not properly validating that the candidate method's return type matches the call site's expected return type.

Key changes:

  • Added return type compatibility checks using both implicit coercion validation and exact type matching
  • Added specialized handling for struct return types with layout and passing convention validation
  • Enhanced logging for debugging incompatible method scenarios

@EgorBo
Copy link
Member Author

EgorBo commented Jul 31, 2025

Reverted to getMethodSig because call->gtRetClsHnd is not set at that point (it's set in impFixupCallStructReturn which is called later). Since delegate devirtualization is a rare event, I guess it's not a big deal to call it here once again.

@EgorBo EgorBo merged commit df9a908 into dotnet:main Aug 1, 2025
108 of 110 checks passed
@EgorBo EgorBo deleted the fix-isCompatibleMethodGDV branch August 1, 2025 09:27
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2025
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.

Parameter Count Mismatch: when using a Delegate during an async / await call via a lambda expression

2 participants