Enable Invoke and GetValue for ref-returning members · Pull Request #17639 · dotnet/coreclr

ghost · 2018-04-18T14:16:19Z

https://github.com/dotnet/corefx/issues/15960

Returned magic object is the object pointed to by
the ref. If the ref is null, NullReferenceException.

https://github.com/dotnet/corefx/issues/15960 Returned magic object is the object pointed to by the ref. If the ref is null, NullReferenceException.

jkotas · 2018-04-18T14:45:02Z

                    //
                    // first take care of all the NO_INVOKE cases. 
                    if (ContainsGenericParameters ||
-                         ReturnType.IsByRef ||


Do we need IsByRefLike condition here as well? Or are the ByRefLike returns handled somewhere else?

That gets handled in the "else" branch (RuntimeMethodHandle.GetSecurityFlags(this);)

jkotas · 2018-04-18T15:12:37Z

                    // first take care of all the NO_INVOKE cases. 
                    if (ContainsGenericParameters ||
-                         ReturnType.IsByRef ||
+                         (ReturnType.IsByRef && ReturnType.GetElementType().IsByRefLike) ||


Update the message that will be thrown for this case in ThrowNoInvokeException ?

jkotas · 2018-04-18T15:14:46Z

+            COMPlusThrow(kNullReferenceException, IDS_INVOKE_NULLREF_RETURNED);
+        }
+
+        // If the target was a value type, that scenario was handled above as part of the code path that copies value types into preallocated boxed objects.


Can we just call InvokeUtil::CreateObject here?

InvokeUtil::CreateObject assumes it's free to allocate objects since the pointer it receives is in non-gc-heap memory. The specific cases we call it for happen not to do that but spreading out the GC restriction over function calls like that leaves the code base in a more fragile state than I'd like to. I went back and forth on this one several times - I decided that the small amount of repetition here was worth paying in order to keep the fragility in that one method.

Ok, there are no other callers for InvokeUtil::CreateObject and it was doing things it didn't need to do so I changed the rules for it and made the byref case use it.

jkotas · 2018-04-18T16:37:41Z

+// CreateObjectAfterInvoke
+// This routine will create the specified object from the value returned by the Invoke target. 
+//
+// This does not handle the ELEMENT_TYPE_VALUETYPE case. There is no way for this function to handle that case in a GC-safe way.


There is no way for this function to handle that case in a GC-safe way.

I do not think that this is valid statement. It can be handled by protecting pValue using GCPROTECT_BEGININTERIOR.

Is that also safe when pValue is a completely unmanaged pointer (i.e. into the stack?)

(I think it is fine for this function to not handle ELEMENT_TYPE_VALUETYPE.)

Ok, we'll delete the phrase from the comment, then.

Is that also safe when pValue is a completely unmanaged pointer (i.e. into the stack?)

Yes.

jkotas · 2018-04-18T16:40:51Z

+            obj = AllocateObject(pMT);
+            memcpyNoGCRefs(obj->UnBox(), &capturedValue, size);
        }
        break;


ELEMENT_TYPE_BYREF should not be needed anymore. Also, it is referring to resource string that no longer exists.

jkotas · 2018-04-18T16:42:40Z

LGTM otherwise. I assume that there are matching corefx tests coming.

jkotas · 2018-04-19T17:43:04Z

@@ -666,6 +671,9 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) {
    MethodTable *pMT = NULL;


GetSignatureCorElementType is a regular element type that you find in metadata signatures, etc.

GetInternalCorElementType used by the caller is relevant for calling convention - what gets passed in registers, etc.

The bug that broke corefx is when these two differ.

Yeah, it broke enums. I'm thinking we should just strip CreateObject down to handling objects, pointers and function pointers and let the prealloc-and-copy code handle all value types.

* Reapply #17639 * tryagain-wip 4/23/2018 7:27:37 AM - Fix Invoke of enum-returning methods * Assert for refbufargs implying valuetype * Catch ref to void in managed layer

@vargaz

…pes (#13901) CoreCLR allows reflection on methods with ByRef return types under certain conditions. The PR implementing it is dotnet/coreclr#17639. Mono already implemented most of it in the marshalling code but artificially blocked it in `ves_icall_InternalInvoke`. I changed the condition for the exception to match the condition in `emit_invoke_call` (marshal-ilgen.c). It's similar to the condition in CoreCLR, but it may not handle byref-like types correctly. It would have to fixed on both places if it turns out to be the case. The remaining issue is that null references returned in ByRef are not checked and converted to correct exception. It should result in `NullReferenceException` with special message that doesn't get wrapped in `TargetInvocationException`. I put a TODO marker at a place where I think it should be handled, but it's a bit over my head to implement it. /cc @vargaz

Enable Invoke and GetValue for ref-returning members

b9eedce

https://github.com/dotnet/corefx/issues/15960 Returned magic object is the object pointed to by the ref. If the ref is null, NullReferenceException.

ghost added the area-System.Reflection label Apr 18, 2018

ghost self-assigned this Apr 18, 2018

dotnet-bot added the 2 - In Progress label Apr 18, 2018

ghost requested a review from jkotas April 18, 2018 14:16

jkotas reviewed Apr 18, 2018

View reviewed changes

atsushikan added 2 commits April 18, 2018 08:29

Fix error message

1ed4fdb

Found way to reuse InvokeUtil::CreateObject

f0a3192

jkotas reviewed Apr 18, 2018

View reviewed changes

Remove comment, BYREF case.

29946f9

ghost merged commit e88dc7f into dotnet:master Apr 18, 2018

dotnet-bot removed the 2 - In Progress label Apr 18, 2018

ghost deleted the ri branch April 18, 2018 20:57

jkotas reviewed Apr 19, 2018

View reviewed changes

ghost pushed a commit that referenced this pull request Apr 19, 2018

Rollback #17639 (#17671)

9361217

filipnavara mentioned this pull request Apr 7, 2019

[netcore] Allow reflection invocation of methods with ByRef return types mono/mono#13901

Merged

lambdageek mentioned this pull request Jun 10, 2019

[netcore] Update the checks for ByRef return type in reflection invocation mono/mono#14967

Merged

This pull request was closed.

		@@ -666,6 +671,9 @@ OBJECTREF InvokeUtil::CreateObject(TypeHandle th, void * pValue) {
		MethodTable *pMT = NULL;

Uh oh!

Conversation

ghost commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkotas Apr 18, 2018 •

edited

Loading

jkotas Apr 18, 2018 •

edited

Loading