Implement object_isinst and object_get_class #129
Conversation
I mixed some extension method additions in here. I found these make discovering the right thing to call easier and cut down on some of the boiler plate that is appearing over and over.
| @@ -55,4 +55,125 @@ public void GCHandleNewAndGetTarget() | |||
| GCHandle.FromIntPtr(handle1).Free(); | |||
| GCHandle.FromIntPtr(handle2).Free(); | |||
| } | |||
|
|
|||
| // Classes and classes | |||
| [TestCase(typeof(object), typeof(object), true)] | |||
There was a problem hiding this comment.
What runtime are these tests running on? Mono? Our CoreCLR with our GC?
There was a problem hiding this comment.
From Rider they are going to run on whatever coreclr rider is using.
From the CI scripts, I'm not sure. Maybe the system installed, maybe our fork. @UnityAlex do you know?
There was a problem hiding this comment.
The Unsafe calls making objects into IntPtr's are actually unsafe ;-) on stock CoreCLR as objects may be moved by the GC.
There was a problem hiding this comment.
From Rider they are going to run on whatever coreclr rider is using.
From the CI scripts, I'm not sure. Maybe the system installed, maybe our fork. @UnityAlex do you know?
Yes the way it's setup currently it's just running dotnet so whatever is on the PATH env var.
There was a problem hiding this comment.
The Unsafe calls making objects into IntPtr's are actually unsafe ;-) on stock CoreCLR as objects may be moved by the GC.
Makes sense. I didn't think about that. I guess we'll see how long we get lucky for 😄
| public static nint TypeHandleIntPtr(this Type type) | ||
| => RuntimeTypeHandle.ToIntPtr(type.TypeHandle); | ||
|
|
||
| public static T UnsafeAs<T>(this nint intPtr) |
There was a problem hiding this comment.
Nit, the extension below is clearly to convert an object to an IntPtr/nint representation in the embedding API. I'm not sure of the best way of organizing it, but we want to make sure we keep all object<->nint and nint<->object conversions in one place (and only use methods for that) because at some point that will switch from just an unsafe cast to allocating/retrieving GC Handles.
There was a problem hiding this comment.
Basically, any objects (reference types) being passes as parameters or as a return value on the embedding interface need special handling.
There was a problem hiding this comment.
Are you suggesting I rename / refactor the methods to be more like
ToNativeRepresentation
FromNativeRepresentation
?
There was a problem hiding this comment.
That's my initial thought. I don't think it's required, just want to ensure we keep those specific operations isolated to we can change them easily without having to re-audit all the code.
It's great. The current setup does not do an end to end test of the native embedding api but it's close enough that I haven't had to work in native code at all yet. No editing of mono_coreclr.cpp or debugging of the native tests. I did start to attempt to debug the native tests but didn't end up needing to, you pointed out the TypeHandle and that was my problem. |
|
This C# test setup is still a bit of an experiment. So far it's been great. If we continue to like it I think we might want to look into seeing if we can source generate methods for calling into our native embedding apis so that we can cover end to end. |
I really like it as well, we may just need some extra work to ensure the tests running on stock CoreCLR are GC safe. But I think it's a goal to keep this approach if possible. |
|
Let me know if you want help or to pair. Given these are just tests, I think we can find a solution. Maybe pinning the managed test we allocate, or wrapping every object in a special wrapper object that we pin. |
|
The main quirk I've hit so far is that Rider runs the source generator as you type. And given how the native source generation is currently setup, where it replaces the existing method, the source generator ends up sprinkling in new methods as you were typing out the managed method name. For example you end up with methods named "object_", "object_isin", and then finally "object_isinst" once you've finished typing out the method name. I discard the changes to "mono_coreclr.cpp" and rebuild managed.sln and then everything is all straightened out. At some point we'll probably switch to source generating after some marker in the file where we know we can discard all changes or we could write to a separate file. Despite this quirk, for now I like the current behavior because it makes the diffs really easy to review. |
Implement
object_isinstandobject_get_classI mixed some extension method additions in here. I found these make discovering the right thing to call easier and cut down on some of the boiler plate that is appearing over and over.