Replace magic offsets with proper offset calculation #1441

lostmsu · 2021-04-12T10:31:57Z

All Python types created to represent CLR concepts derive from CLR MetaType (as before), which now has two new fields:

tp_clr_inst_offset, which is similar to tp_dictoffset in that it tells where to find GCHandle in instances of this type (e.g. where to find GCHandle pointing to System.Uri in corresponding Python object)
tp_clr_inst, which holds an optional instance of ManagedType, that implements the behavior of the type itself (e.g. GCHandle pointing to ClassObject(type = System.Uri))

So the layout of all Python types created by Python.NET is

  PyType type;
  nint tp_clr_inst_offset;
  GCHandle tp_clr_inst; // points, for example, to an instance of `ClassObject`

When a Python type, that reflects CLR type is created (such as <class 'System.Uri'>), the layout of its instances will be:

  BaseTypeFields base;
  /*if not already in base*/ IntPtr dict;
  /*if not already in base*/ IntPtr weaklist;
  /*if not already in base*/ GCHandle gcHandle; 
  // gcHandle points to `CLRObject` instance, which in turn, for example, points to the actual instance of `System.Uri`

The offset to GC handle is recorded in the Python type's tp_clr_inst_offset, and can be found as PyObject_Type(inst).tp_clr_inst_offset. Or, preferably, accessor functions in ManagedType should be used, e.g. ManagedType.SetGCHandle(pyObj, gcHandle).

Still uses TypeOffset until PyType use is widespread.

Notes

Also removed some macro-like method copy-pastes from CPython and bits of dead code.

Should not affect observed behavior.

lostmsu · 2021-04-14T00:40:47Z

@amos402 can you take a look?

BadSingleton

Mostly comments about error handling. I never quite understood the memory layout that was used before this PR, the new one makes sense, but I am not sure I grasp all the implications.

src/runtime/extensiontype.cs

BadSingleton · 2021-04-21T13:28:56Z

src/runtime/clrobject.cs

@@ -78,6 +68,9 @@ internal static IntPtr GetInstHandle(object ob)
            return co.pyHandle;
        }

+        internal static NewReference GetReference(object ob)


Should this throw if ob is null? (maybe debug only)

It does raise a NullReferenceException just down the line. I think it is fine not to check here.

BadSingleton · 2021-04-21T13:38:38Z

src/runtime/extensiontype.cs

        }

-        void SetupGc ()
+        void SetupGc (bool force)


Looking at the code in the function, I'm not sure "force" is the right word? Maybe this method needs to be split in two? (or inlined code since it's used in two places)

Good point. I replaced this with a debug only check before the call.

BadSingleton · 2021-04-21T13:41:59Z

src/runtime/importhook.cs

@@ -90,8 +70,7 @@ internal static unsafe void Initialize()
            root = new CLRModule();

            // create a python module with the same methods as the clr module-like object
-            InitializeModuleDef();
-            py_clr_module = Runtime.PyModule_Create2(module_def, 3);
+            py_clr_module = Runtime.PyModule_New("clr").DangerousMoveToPointer();


I'm not entirely sure of the internal differences with PyModule_Create2 but the documentation states that the caller is responsible to set a __file__ attribute. Maybe not necessary though.

__file__ is optional. I don't think we have a file name to provide here.

src/runtime/interop.cs

BadSingleton · 2021-04-21T17:13:11Z

src/runtime/metatype.cs

@@ -165,9 +163,17 @@ public static IntPtr tp_new(IntPtr tp, IntPtr args, IntPtr kw)
            TypeManager.CopySlot(base_type, type, TypeOffset.tp_clear);


+            int clrInstOffset = Marshal.ReadInt32(base_type, Offsets.tp_clr_inst_offset);
+            Marshal.WriteInt32(type, Offsets.tp_clr_inst_offset, clrInstOffset);
+
            // for now, move up hidden handle...


nit: I think this comment should say something like "copy the instance GC from the base to the derived class"

BadSingleton · 2021-04-21T17:16:26Z

src/runtime/metatype.cs

-                ((GCHandle)gc).Free();
+                GetGCHandle(new BorrowedReference(tp)).Free();
+#if DEBUG
+                SetGCHandle(new BorrowedReference(tp), Runtime.CLRMetaType, default);


Why set it to a reasonable value in debug? I'd be more inclined to make it crash and/or throw an exception.

It is a legitimate question, and since took me a while to understand why I put it here and then forgot.

What it does in Debug build is that instead of CLR crash (typically ExecutionEngineException) you might get when accessing bad GCHandle it raises a debuggable exception, that you might notice when running tests in debug build.

To never forget again, a comment would be good.

BadSingleton · 2021-04-21T17:18:18Z

src/runtime/typemanager.cs

            if (clrType == typeof(Exception))
            {
                base_ = Exceptions.Exception;
            }
            else if (clrType.BaseType != null)
            {
                ClassBase bc = ClassManager.GetClass(clrType.BaseType);
-                base_ = bc.pyHandle;
+                if (bc.ObjectReference != null)


Shouldn't this assert in debug? Or even throw an error in non-debug builds?

Now, this is for the rare scenario with circular initialization order. We have a test covering it, that prompted me to add the if in the first place.

Then a comment would be good.

BadSingleton · 2021-04-21T17:53:01Z

src/runtime/classbase.cs

-            self.FreeGCHandle();
+            Runtime.PyObject_GC_UnTrack(ob);
+            Runtime.PyObject_GC_Del(ob);
+            self?.FreeGCHandle();


Is this expected?/Should this be an error in debug?

I've seen a scenario or two when this happens after tp_clear is called. Same as below.

BadSingleton · 2021-04-21T17:54:02Z

src/runtime/classbase.cs

            {
                ClearObjectDict(ob);
            }
-            self.tpHandle = IntPtr.Zero;
+            if (self is not null) self.tpHandle = IntPtr.Zero;


same as above

lostmsu · 2021-04-22T05:24:18Z

@BadSingleton addressed your comments.

BadSingleton

Looks good to me, again, I would like another reviewer to also take a look.

BadSingleton · 2021-04-23T18:45:54Z

src/runtime/typemanager.cs

            if (clrType == typeof(Exception))
            {
                base_ = Exceptions.Exception;
            }
            else if (clrType.BaseType != null)
            {
                ClassBase bc = ClassManager.GetClass(clrType.BaseType);
-                base_ = bc.pyHandle;
+                if (bc.ObjectReference != null)


Then a comment would be good.

BadSingleton · 2021-04-23T18:47:16Z

src/runtime/metatype.cs

-                ((GCHandle)gc).Free();
+                GetGCHandle(new BorrowedReference(tp)).Free();
+#if DEBUG
+                SetGCHandle(new BorrowedReference(tp), Runtime.CLRMetaType, default);


To never forget again, a comment would be good.

removed some macro-like method copy-pastes from CPython and bits of dead code All Python types created to represent CLR concepts derive from `CLR MetaType` (as before), which now has two new fields: - `tp_clr_inst_offset`, which is similar to `tp_dictoffset` in that it tells where to find `GCHandle` in instances of this type (e.g. where to find `GCHandle` pointing to `System.Uri` in corresponding Python object) - `tp_clr_inst`, which holds an optional instance of `ManagedType`, that implements the behavior of the type itself (e.g. `GCHandle` pointing to `ClassObject(type = System.Uri)`) So the layout of all Python types created by Python.NET is PyType type; nint tp_clr_inst_offset; GCHandel tp_clr_inst; (points, for example, to an instance of `ClassObject`) When a Python type, that reflects CLR type is created, the layout of instances will be: BaseTypeFields base; optional (if not in base): IntPtr dict; optional (if not in base): IntPtr weaklist; GCHandle gcHandle; (points to `CLRObject` instance, which in turn, for example, points to the actual instance of `System.Uri`) The offset to GC handle is recorded in the Python type's `tp_clr_inst_offset`, and can be found as `PyObject_Type(inst).tp_clr_inst_offset`. Or, preferably, accessor functions in `ManagedType` should be used.

lostmsu force-pushed the PR/NoMagic branch 2 times, most recently from f788b6a to bc3ba6a Compare April 12, 2021 22:46

lostmsu marked this pull request as ready for review April 12, 2021 22:51

lostmsu requested a review from filmor April 12, 2021 22:57

lostmsu force-pushed the PR/NoMagic branch from bc3ba6a to 30ffeab Compare April 13, 2021 00:43

lostmsu mentioned this pull request Apr 13, 2021

Unable to run with Python debug library #1412

Closed

BadSingleton reviewed Apr 21, 2021

View reviewed changes

lostmsu force-pushed the PR/NoMagic branch 2 times, most recently from 936bff4 to e50e1d2 Compare April 22, 2021 05:23

BadSingleton approved these changes Apr 23, 2021

View reviewed changes

lostmsu force-pushed the PR/NoMagic branch from e50e1d2 to b2a911d Compare April 23, 2021 21:55

lostmsu merged commit daccc43 into pythonnet:master Apr 24, 2021

lostmsu deleted the PR/NoMagic branch April 24, 2021 00:21

lostmsu mentioned this pull request May 3, 2021

Removed unnecessary dictionary initialization in CLRObject #1413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace magic offsets with proper offset calculation #1441

Replace magic offsets with proper offset calculation #1441

lostmsu commented Apr 12, 2021 •

edited

Loading

lostmsu commented Apr 14, 2021

BadSingleton left a comment

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 21, 2021

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 23, 2021

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 23, 2021

BadSingleton Apr 21, 2021

lostmsu Apr 22, 2021

BadSingleton Apr 21, 2021

lostmsu commented Apr 22, 2021

BadSingleton left a comment

BadSingleton Apr 23, 2021

BadSingleton Apr 23, 2021

Replace magic offsets with proper offset calculation #1441

Replace magic offsets with proper offset calculation #1441

Conversation

lostmsu commented Apr 12, 2021 • edited Loading

Related

Notes

lostmsu commented Apr 14, 2021

BadSingleton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostmsu commented Apr 22, 2021

BadSingleton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostmsu commented Apr 12, 2021 •

edited

Loading