[mini] Don't add unbox tramopline on generic DIM calls #58521
Conversation
ghost
added
the
|
Tagging subscribers to this area: Issue DetailsDon't unbox a valuetype Fixes #58394
|
Don't unbox a valuetype `this` if the generic method is a DIM Fixes dotnet#58394
lambdageek
force-pushed
the
fix-gh-58394
branch
3 times, most recently
from
600cabb to
f46f4e6
Compare
lambdageek
force-pushed
the
fix-gh-58394
branch
from
f46f4e6 to
e9bd9d4
Compare
lambdageek
requested review from
SamMonoRT,
imhameed and
vargaz
as code owners
| } else if (orig_vtable_slot) { | ||
| if (m_class_is_valuetype (m->klass)) | ||
| if (m_class_is_valuetype (m->klass)) { | ||
| g_assert (!mini_method_is_default_method (m)); |
There was a problem hiding this comment.
Why don't you add the same condition as above?
There was a problem hiding this comment.
Good question.
I couldn't tell if we could ever end up in this branch with a valuetype klass and a default interface method.
I thought it would be better to add an assertion (because if it does ever happen the failure will be right at this spot) instead of skipping the need_unbox_tramp. If we erroneous add or don't add an unbox trampoline, we will get a crash later when we actually call the underlying managed method and it uses this in some way. That will be a crash in some unrelated managed code and it won't be obvious how to trace it back to this trampoline. With the assert if anyone comes up with a way to violate it the assumptions it will break in the place where we need to fix it.
There was a problem hiding this comment.
That's right. The assertion would definitely make debugging easier. But my confusion is that in the future, when someone hit this assertion, I feel the solution would be to skip adding unbox trampoline. Or you think more work would be needed on top of that?
There was a problem hiding this comment.
That's right. The assertion would definitely make debugging easier. But my confusion is that in the future, when someone hit this assertion, I feel the solution would be to skip adding unbox trampoline. Or you think more work would be needed on top of that?
I don't know. I can't tell from the code how we end up in this branch with a DIM. I'd need to see an example and I couldn't make one.
And also I can't tell if in that case the right thing would be to add an unbox, or to not add one. To be honest I was hoping one of our existing tests would break on the assert, and then I'd know that things were fine as is.
But nothing broke. So I still don't know whether that case can happen. And what we should do if it does happen.
|
We need to backport to release\6.0 once all validation is complete |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1195616255 |
|
/azp run sync-runtime-to-mono |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
Don't unbox a valuetype
thisif the generic method is a DIMFixes #58394