-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[cdac] Remove GetMethodDescData fallback to legacy DAC #110702
[cdac] Remove GetMethodDescData fallback to legacy DAC #110702
Conversation
Tagging subscribers to this area: @tommcdon |
Do you think it would be beneficial to verify that the HResult is the same for cDAC/DAC in debug builds? Previously this didn't matter as if the cDAC returned Instead of immediately returning on failure, we could call the fallback and assert the same result code, then return the cDAC result. |
I think that makes sense for the error cases that aren't just basic input checks. I'll switch the various debug-only checks we have against for cDAC/DAC match to compare the HRESULTs if cDAC throws/catches an exception. |
@@ -264,7 +264,7 @@ public TypeHandle GetTypeHandle(TargetPointer typeHandlePointer) | |||
|
|||
if ((addressLowBits != TypeHandleBits.MethodTable) && (addressLowBits != TypeHandleBits.TypeDesc)) | |||
{ | |||
throw new InvalidOperationException("Invalid type handle pointer"); | |||
throw new ArgumentException("Invalid type handle pointer", nameof(typeHandlePointer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched these to match the HRESULT returned by the DAC. Found via @max-charlamb's suggestion in #110702 (comment)
After #110322 goes in, the cDAC
GetMethodDescData
implementation should be done, so we can stop delegating to the legacy DAC as a fallback.Contributes to #109426.