- Notifications
You must be signed in to change notification settings - Fork749
Replace magic offsets with proper offset calculation#1441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f788b6a tobc3ba6aCompare@amos402 can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -78,6 +68,9 @@ internal static IntPtr GetInstHandle(object ob) | |||
| return co.pyHandle; | |||
| } | |||
| internal static NewReference GetReference(object ob) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this throw if ob is null? (maybe debug only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It does raise aNullReferenceException just down the line. I think it is fine not to check here.
src/runtime/extensiontype.cs Outdated
| } | ||
| void SetupGc () | ||
| void SetupGc (bool force) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point. I replaced this with a debug only check before the call.
| @@ -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(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not entirely sure of the internal differences withPyModule_Create2 but the documentation states that the caller is responsible to set a__file__ attribute. Maybe not necessary though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
__file__is optional. I don't think we have a file name to provide here.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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... | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: I think this comment should say something like "copy the instance GC from the base to the derived class"
| ((GCHandle)gc).Free(); | ||
| GetGCHandle(new BorrowedReference(tp)).Free(); | ||
| #if DEBUG | ||
| SetGCHandle(new BorrowedReference(tp), Runtime.CLRMetaType, default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why set it to a reasonable value in debug? I'd be more inclined to make it crash and/or throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 (typicallyExecutionEngineException) you might get when accessing badGCHandle it raises a debuggable exception, that you might notice when running tests in debug build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To never forget again, a comment would be good.
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't this assert in debug? Or even throw an error in non-debug builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Now, this is for the rare scenario with circular initialization order. We have a test covering it, that prompted me to add theif in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then a comment would be good.
| self.FreeGCHandle(); | ||
| Runtime.PyObject_GC_UnTrack(ob); | ||
| Runtime.PyObject_GC_Del(ob); | ||
| self?.FreeGCHandle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this expected?/Should this be an error in debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've seen a scenario or two when this happens aftertp_clear is called. Same as below.
| { | ||
| ClearObjectDict(ob); | ||
| } | ||
| self.tpHandle = IntPtr.Zero; | ||
| if (self is not null)self.tpHandle = IntPtr.Zero; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above
936bff4 toe50e1d2Compare@BadSingleton addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me, again, I would like another reviewer to also take a look.
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then a comment would be good.
| ((GCHandle)gc).Free(); | ||
| GetGCHandle(new BorrowedReference(tp)).Free(); | ||
| #if DEBUG | ||
| SetGCHandle(new BorrowedReference(tp), Runtime.CLRMetaType, default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To never forget again, a comment would be good.
removed some macro-like method copy-pastes from CPython and bits of dead codeAll 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.
Uh oh!
There was an error while loading.Please reload this page.
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 totp_dictoffsetin that it tells where to findGCHandlein instances of this type (e.g. where to findGCHandlepointing toSystem.Uriin corresponding Python object)tp_clr_inst, which holds an optional instance ofManagedType, that implements the behavior of the type itself (e.g.GCHandlepointing toClassObject(type = System.Uri))So the layout of all Python types created by Python.NET is
When a Python type, that reflects CLR type is created (such as
<class 'System.Uri'>), the layout of itsinstances will be:The offset to GC handle is recorded in the Python type's
tp_clr_inst_offset, and can be found asPyObject_Type(inst).tp_clr_inst_offset. Or, preferably, accessor functions inManagedTypeshould be used, e.g.ManagedType.SetGCHandle(pyObj, gcHandle).Still uses
TypeOffsetuntilPyTypeuse is widespread.Related
This should (at least partially) supersede#1267
Notes
Also removed some macro-like method copy-pastes from CPython and bits of dead code.
Should not affect observed behavior.