Skip to content

Replace MethodDescCallSite with UnmanagedCallersOnly in clrex.cpp#126061

Open
AaronRobinsonMSFT wants to merge 2 commits intodotnet:mainfrom
AaronRobinsonMSFT:dev/arobins/clrex-uco-conversion
Open

Replace MethodDescCallSite with UnmanagedCallersOnly in clrex.cpp#126061
AaronRobinsonMSFT wants to merge 2 commits intodotnet:mainfrom
AaronRobinsonMSFT:dev/arobins/clrex-uco-conversion

Conversation

@AaronRobinsonMSFT
Copy link
Member

Note

This PR description was generated with the assistance of GitHub Copilot.

Convert exception construction in clrex.cpp from MethodDescCallSite/CallDescrWorker to the UnmanagedCallersOnlyCaller pattern for EEArgumentException, EETypeLoadException, and EEFileLoadException.

Changes

  • Exception.CoreCLR.cs - Add [UnmanagedCallersOnly] CreateArgumentException factory method
  • FileLoadException.CoreCLR.cs - Add [UnmanagedCallersOnly] Create factory method
  • TypeLoadException.CoreCLR.cs - Add [UnmanagedCallersOnly] Create factory method
  • clrex.cpp - Replace MethodDescCallSite usage with UnmanagedCallersOnlyCaller in three CreateThrowable methods
  • corelib.h - Register new managed methods (METHOD__EXCEPTION__CREATE_ARGUMENT_EXCEPTION, METHOD__TYPE_LOAD_EXCEPTION__CREATE, METHOD__FILE_LOAD_EXCEPTION__CREATE)
  • metasig.h - Remove unused metasig entries (IM(Str_Int_RetVoid), IM(Str_Str_Str_Int_RetVoid))

Contributes to #123864

Convert EEArgumentException, EETypeLoadException, and EEFileLoadException
CreateThrowable methods to use UnmanagedCallersOnlyCaller instead of
MethodDescCallSite/CallDescrWorker for invoking managed exception
constructors. Add corresponding [UnmanagedCallersOnly] factory methods
in managed code and remove unused metasig entries.

Contributes to dotnet#123864

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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 migrates several VM-to-managed exception construction paths in src/coreclr/vm/clrex.cpp from MethodDescCallSite / CallDescrWorker to the UnmanagedCallersOnlyCaller reverse P/Invoke pattern, adding the corresponding [UnmanagedCallersOnly] managed factory entrypoints and binder registrations in CoreLib.

Changes:

  • Add [UnmanagedCallersOnly] managed factory methods for ArgumentException, TypeLoadException, and FileLoadException construction.
  • Update clrex.cpp to invoke these factories via UnmanagedCallersOnlyCaller instead of MethodDescCallSite.
  • Register new CoreLib binder method IDs and remove now-unused metasig entries.

Reviewed changes

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

Show a summary per file
File Description
src/coreclr/vm/metasig.h Removes metasig entries no longer needed after dropping MethodDescCallSite ctor invocations.
src/coreclr/vm/corelib.h Adds binder registrations for the new managed [UnmanagedCallersOnly] factory methods.
src/coreclr/vm/clrex.cpp Switches three CreateThrowable() implementations to UnmanagedCallersOnlyCaller.
src/coreclr/System.Private.CoreLib/src/System/TypeLoadException.CoreCLR.cs Adds [UnmanagedCallersOnly] TypeLoadException.Create factory.
src/coreclr/System.Private.CoreLib/src/System/IO/FileLoadException.CoreCLR.cs Adds [UnmanagedCallersOnly] FileLoadException.Create factory.
src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs Adds [UnmanagedCallersOnly] Exception.CreateArgumentException helper for ctor invocation.

Refactor EEFileLoadException::GetFileLoadKind to use a switch statement.
Add ArgumentExceptionKind and FileLoadExceptionKind enums for managed mapping.
Update CreateThrowable methods to use UnmanagedCallersOnlyCaller.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines +59 to +60
FileLoadExceptionKind.FileLoad => new FileLoadException(fileName, hresult),
_ => throw new InvalidOperationException()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FileLoadExceptionKind.FileLoad => new FileLoadException(fileName, hresult),
_ => throw new InvalidOperationException()
_ /* FileLoadExceptionKind.FileLoad */ => new FileLoadException(fileName, hresult),

try
{
string? fileName = pFileName is not null ? new string(pFileName) : null;
*pThrowable = kind switch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*pThrowable = kind switch
Debug.Assert(Enum.IsDefined(kind));
*pThrowable = kind switch


// Note that ArgumentException takes arguments to its constructor in a different order,
// for usability reasons. However it is inconsistent with our other exceptions.
*pThrowable = kind switch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*pThrowable = kind switch
Debug.Assert(Enum.IsDefined(kind));
*pThrowable = kind switch

Comment on lines +420 to +423
ArgumentExceptionKind.Argument => new ArgumentException(message, paramName),
ArgumentExceptionKind.ArgumentNull => new ArgumentNullException(paramName, message),
ArgumentExceptionKind.ArgumentOutOfRange => new ArgumentOutOfRangeException(paramName, message),
_ => throw new InvalidOperationException()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ArgumentExceptionKind.Argument => new ArgumentException(message, paramName),
ArgumentExceptionKind.ArgumentNull => new ArgumentNullException(paramName, message),
ArgumentExceptionKind.ArgumentOutOfRange => new ArgumentOutOfRangeException(paramName, message),
_ => throw new InvalidOperationException()
ArgumentExceptionKind.ArgumentNull => new ArgumentNullException(paramName, message),
ArgumentExceptionKind.ArgumentOutOfRange => new ArgumentOutOfRangeException(paramName, message),
_ /* ArgumentExceptionKind.Argument */ => new ArgumentException(message, paramName)

Comment on lines +416 to +417
// Note that ArgumentException takes arguments to its constructor in a different order,
// for usability reasons. However it is inconsistent with our other exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note that ArgumentException takes arguments to its constructor in a different order,
// for usability reasons. However it is inconsistent with our other exceptions.

I do not think we need this comment here

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants