Enable Invoke and GetValue for ref-returning members#17732
Conversation
ghost
added
the
ghost
requested a review
from 
| refReturnTargetTH = retTH.AsTypeDesc()->GetTypeParam(); | ||
|
|
||
| // If the target of the byref is a value type, we need to preallocate a boxed object to hold the managed return value. | ||
| if (refReturnTargetTH.IsValueType()) |
There was a problem hiding this comment.
Can we get here with ref void? I know that you cannot type ref void, but it may be possible in IL.
There was a problem hiding this comment.
Yes, it's possible in IL and when I try it, the Invoke succeeds (and calls the method) and gives me a boxed Void object.
I think the runtime loader or JIT should have raised an objection?
There was a problem hiding this comment.
GetMethodTable()->Allocate() does not check whether it is ok to allocate instance the MethodTable. It assumes that the caller verified it.
I think a similar problem can happen for ref Span<char>.
There was a problem hiding this comment.
I meant that the "ref void" method itself shouldn't ever have gotten past the JIT phase. "ref Span" is a legal method - just not invokable by a boxing api. Reflection does screen for ref-to-byreflike so we're covered there. We could screen for ref to void in the same place but it doesn't seem like the right place to check that - "ref void" just shouldn't be allowed.
There was a problem hiding this comment.
This is the endless discussion whether ref X is just X* with added GC tracking, or whether it has special restrictions attached to it (cannot be null, cannot point to void, ...).
IL treats it like X* with added GC tracking. ref void was always allowed in IL. I do not see a good reason to introduce a breaking change to disallow it.
C# has special restrictions for it.
This dichotomy has always been a source of confusion.
| if (retType == ELEMENT_TYPE_VALUETYPE || fHasRetBuffArg) { | ||
| CorElementType retType = retTH.GetSignatureCorElementType(); | ||
| BOOL hasValueTypeReturn = retTH.IsValueType() && retType != ELEMENT_TYPE_VOID; | ||
| if (hasValueTypeReturn || fHasRetBuffArg) { |
There was a problem hiding this comment.
I do not think that the code would handle fHasRetBuffArg=true and hasValueTypeReturn=false combination correctly. This combination can never happen today. Add assert for it, and simplify the condition to just if (hasValueTypeReturn)?
| } | ||
| else | ||
| if (retType == ELEMENT_TYPE_VALUETYPE) | ||
| if (hasValueTypeReturn || hasRefReturnAndNeedsBoxing) |
There was a problem hiding this comment.
No, the || hasRefReturnAndNeedsBoxing has to stay since the if clause now contains the logic for the hasRefReturnAndNeedsBoxing handling as well. (I did this way to avoid creating another duplicate Nullable::NormalizeBox call along with its associated TODO).
|
Does this need special treatment for ref returning unmanaged pointer types? |
That gets handled in |
|
Any further feedback? CI's green now except for the bogus WIP item (only because one of the intermediate commits had "WIP" in its commit message.) |
|
The CI did not run much for some reason. |
|
@dotnet-bot test this please |
|
Not sure what is going on. The CI should have around 15 jobs... |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
This is a port of dotnet/coreclr#17732. Fixes bug 603305. Per popular demand from people who would like to serialize ref returning properties or use them in data binding, adding support for this in MethodInfo.Invoke. The choice was to make these dereference the result and return it like that. For the corner case of returning a ref to a null, a `NullReferenceException` will be thrown (this behavior has a very unfortunate side effect of slowing down the general Invoke path because we can't throw this exception from a place where we would end up wrapping this in a TargetInvocationException). I also made tweaks to byref parameters path to fix byref pointer parameters, but this is horribly broken on the CLR so I didn't bother to test it here. Should work. Also includes a slight size on disk optimization that merges common tails of the static/instance paths of the invoker thunk. Testing of the calling convention converter portion was done by disabling invoke thunks generation in NUTC and running UnitTests. I also ran it with GCStresss (RH_GCStressThrottleMode=1, RH_GCStress_Mode=1). Let me know if there's more testing needed. This has a huge test matrix for being such a corner case thing. [tfs-changeset: 1709919]
4 participants
https://github.com/dotnet/corefx/issues/15960
(Attempt #2)
Returned magic object is the object pointed to by
the ref. If the ref is null, NullReferenceException.