8000 Replace magic offsets with proper offset calculation by lostmsu · Pull Request #1441 · pythonnet/pythonnet · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2021
Merged

Replace magic offsets with proper offset calculation #1441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/embed_tests/TestPyType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Runtime.InteropServices;
using System.Text;

using NUnit.Framework;
Expand Down Expand Up @@ -30,7 +31,7 @@ public void CanCreateHeapType()
using var doc = new StrPtr(docStr, Encoding.UTF8);
var spec = new TypeSpec(
name: name,
basicSize: ObjectOffset.Size(Runtime.Runtime.PyTypeType),
basicSize: Marshal.ReadInt32(Runtime.Runtime.PyBaseObjectType, TypeOffset.tp_basicsize),
slots: new TypeSpec.Slot[] {
new (TypeSlotID.tp_doc, doc.RawPointer),
},
Expand Down
17 changes: 8 additions & 9 deletions src/runtime/classbase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Runtime.Serialization;

namespace Python.Runtime
{
Expand Down Expand Up @@ -355,19 +352,21 @@ public static void tp_dealloc(IntPtr ob)
{
ManagedType self = GetManagedObject(ob);
tp_clear(ob);
Runtime.PyObject_GC_UnTrack(self.pyHandle);
Runtime.PyObject_GC_Del(self.pyHandle);
self.FreeGCHandle();
Runtime.PyObject_GC_UnTrack(ob);
Runtime.PyObject_GC_Del(ob);
self?.FreeGCHandle();
Copy link
Contributor

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?

Copy link
Member Author

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 after tp_clear is called. Same as below.

}

public static int tp_clear(IntPtr ob)
{
ManagedType self = GetManagedObject(ob);
if (!self.IsTypeObject())

bool isTypeObject = Runtime.PyObject_TYPE(ob) == Runtime.PyCLRMetaType;
if (!isTypeObject)
{
ClearObjectDict(ob);
}
self.tpHandle = IntPtr.Zero;
if (self is not null) self.tpHandle = IntPtr.Zero;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

return 0;
}

Expand All @@ -391,7 +390,7 @@ protected override void OnLoad(InterDomainContext context)
SetObjectDict(pyHandle, dict);
}
gcHandle = AllocGCHandle();
Marshal.WriteIntPtr(pyHandle, TypeOffset.magic(), (IntPtr)gcHandle);
SetGCHandle(ObjectReference, gcHandle);
}


Expand Down
12 changes: 4 additions & 8 deletions src/runtime/classderived.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
Expand Down Expand Up @@ -75,8 +76,8 @@ internal ClassDerivedObject(Type tp) : base(tp)
// So we don't call PyObject_GC_Del here and instead we set the python
// reference to a weak reference so that the C# object can be collected.
GCHandle gc = GCHandle.Alloc(self, GCHandleType.Weak);
int gcOffset = ObjectOffset.magic(Runtime.PyObject_TYPE(self.pyHandle));
Marshal.WriteIntPtr(self.pyHandle, gcOffset, (IntPtr)gc);
Debug.Assert(self.TypeReference == Runtime.PyObject_TYPE(self.ObjectReference));
SetGCHandle(self.ObjectReference, self.TypeReference, gc);
self.gcHandle.Free();
self.gcHandle = gc;
}
Expand Down Expand Up @@ -106,7 +107,7 @@ internal static IntPtr ToPython(IPythonDerivedType obj)
Runtime._Py_NewReference(self.pyHandle);
#endif
GCHandle gc = GCHandle.Alloc(self, GCHandleType.Normal);
Marshal.WriteIntPtr(self.pyHandle, ObjectOffset.magic(self.tpHandle), (IntPtr)gc);
SetGCHandle(self.ObjectReference, self.TypeReference, gc);
self.gcHandle.Free();
self.gcHandle = gc;

Expand Down Expand Up @@ -883,11 +884,6 @@ public static void Finalize(IPythonDerivedType obj)
// the C# object is being destroyed which must mean there are no more
// references to the Python object as well so now we can dealloc the
// python object.
IntPtr dict = Marshal.ReadIntPtr(self.pyHandle, ObjectOffset.TypeDictOffset(self.tpHandle));
if (dict != IntPtr.Zero)
{
Runtime.XDecref(dict);
}
Runtime.PyObject_GC_Del(self.pyHandle);
self.gcHandle.Free();
}
Expand Down
23 changes: 8 additions & 15 deletions src/runtime/clrobject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,16 @@ internal CLRObject(object ob, IntPtr tp)
System.Diagnostics.Debug.Assert(tp != IntPtr.Zero);
IntPtr py = Runtime.PyType_GenericAlloc(tp, 0);

var flags = (TypeFlags)Util.ReadCLong(tp, TypeOffset.tp_flags);
if ((flags & TypeFlags.Subclass) != 0)
{
IntPtr dict = Marshal.ReadIntPtr(py, ObjectOffset.TypeDictOffset(tp));
if (dict == IntPtr.Zero)
{
dict = Runtime.PyDict_New();
Marshal.WriteIntPtr(py, ObjectOffset.TypeDictOffset(tp), dict);
}
}

GCHandle gc = AllocGCHandle(TrackTypes.Wrapper);
Marshal.WriteIntPtr(py, ObjectOffset.magic(tp), (IntPtr)gc);
tpHandle = tp;
pyHandle = py;
inst = ob;

GCHandle gc = AllocGCHandle(TrackTypes.Wrapper);
InitGCHandle(ObjectReference, type: TypeReference, gc);

// Fix the BaseException args (and __cause__ in case of Python 3)
// slot if wrapping a CLR exception
Exceptions.SetArgsAndCause(py);
if (ob is Exception e) Exceptions.SetArgsAndCause(e, py);
}

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

internal static NewReference GetReference(object ob)
Copy link
Contributor

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

=> NewReference.DangerousFromPointer(GetInstHandle(ob));

internal static CLRObject Restore(object ob, IntPtr pyHandle, InterDomainContext context)
{
CLRObject co = new CLRObject()
Expand All @@ -101,7 +94,7 @@ protected override void OnLoad(InterDomainContext context)
{
base.OnLoad(context);
GCHandle gc = AllocGCHandle(TrackTypes.Wrapper);
Marshal.WriteIntPtr(pyHandle, ObjectOffset.magic(tpHandle), (IntPtr)gc);
SetGCHandle(ObjectReference, TypeReference, gc);
}
}
}
6 changes: 3 additions & 3 deletions src/runtime/converter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ private static bool ToPrimitive(IntPtr value, Type obType, out object result, bo
{
if (Runtime.PyBytes_Size(value) == 1)
{
op = Runtime.PyBytes_AS_STRING(value);
op = Runtime.PyBytes_AsString(value);
result = (byte)Marshal.ReadByte(op);
return true;
}
Expand All @@ -606,7 +606,7 @@ private static bool ToPrimitive(IntPtr value, Type obType, out object result, bo
{
if (Runtime.PyBytes_Size(value) == 1)
{
op = Runtime.PyBytes_AS_STRING(value);
op = Runtime.PyBytes_AsString(value);
result = (byte)Marshal.ReadByte(op);
return true;
}
Expand All @@ -632,7 +632,7 @@ private static bool ToPrimitive(IntPtr value, Type obType, out object result, bo
{
if (Runtime.PyBytes_Size(value) == 1)
{
op = Runtime.PyBytes_AS_STRING(value);
op = Runtime.PyBytes_AsString(value);
result = (byte)Marshal.ReadByte(op);
return true;
}
Expand Down
16 changes: 5 additions & 11 deletions src/runtime/exceptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,8 @@ internal static void Shutdown()
/// pointer.
/// </summary>
/// <param name="ob">The python object wrapping </param>
internal static void SetArgsAndCause(IntPtr ob)
internal static void SetArgsAndCause(Exception e, IntPtr ob)
{
// e: A CLR Exception
Exception e = ExceptionClassObject.ToException(ob);
if (e == null)
{
return;
}

IntPtr args;
if (!string.IsNullOrEmpty(e.Message))
{
Expand All @@ -177,13 +170,14 @@ internal static void SetArgsAndCause(IntPtr ob)
args = Runtime.PyTuple_New(0);
}

Marshal.WriteIntPtr(ob, ExceptionOffset.args, args);
if (Runtime.PyObject_SetAttrString(ob, "args", args) != 0)
throw new PythonException();

if (e.InnerException != null)
{
// Note: For an AggregateException, InnerException is only the first of the InnerExceptions.
IntPtr cause = CLRObject.GetInstHandle(e.InnerException);
Marshal.WriteIntPtr(ob, ExceptionOffset.cause, cause);
using var cause = CLRObject.GetReference(e.InnerException);
Runtime.PyException_SetCause(ob, cause.DangerousMoveToPointer());
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/runtime/extensiontype.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,17 @@ public ExtensionType()
tpHandle = tp;
pyHandle = py;

#if DEBUG
GetGCHandle(ObjectReference, TypeReference, out var existing);
System.Diagnostics.Debug.Assert(existing == IntPtr.Zero);
#endif
SetupGc();
}

void SetupGc ()
{
GCHandle gc = AllocGCHandle(TrackTypes.Extension);
Marshal.WriteIntPtr(pyHandle, ObjectOffset.magic(tpHandle), (IntPtr)gc);
InitGCHandle(ObjectReference, TypeReference, gc);

// We have to support gc because the type machinery makes it very
// hard not to - but we really don't have a need for it in most
Expand Down
28 changes: 1 addition & 27 deletions src/runtime/importhook.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
F438 Expand Up @@ -15,26 +15,6 @@ internal static class ImportHook
private static IntPtr py_clr_module;
static BorrowedReference ClrModuleReference => new BorrowedReference(py_clr_module);

private static IntPtr module_def = IntPtr.Zero;

internal static void InitializeModuleDef()
{
if (module_def == IntPtr.Zero)
{
module_def = ModuleDefOffset.AllocModuleDef("clr");
}
}

internal static void ReleaseModuleDef()
{
if (module_def == IntPtr.Zero)
{
return;
}
ModuleDefOffset.FreeModuleDef(module_def);
module_def = IntPtr.Zero;
}

/// <summary>
/// Initialize just the __import__ hook itself.
/// </summary>
Expand Down Expand Up @@ -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();
Copy link
Contributor

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 with PyModule_Create2 but the documentation states that the caller is responsible to set a __file__ attribute. Maybe not necessary though.

Copy link
Member Author

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.


// both dicts are borrowed references
BorrowedReference mod_dict = Runtime.PyModule_GetDict(ClrModuleReference);
Expand All @@ -116,13 +95,8 @@ internal static void Shutdown()

RestoreImport();

bool shouldFreeDef = Runtime.Refcount(py_clr_module) == 1;
Runtime.XDecref(py_clr_module);
py_clr_module = IntPtr.Zero;
if (shouldFreeDef)
{
ReleaseModuleDef();
}

Runtime.XDecref(root.pyHandle);
root = null;
Expand Down
Loading
0