Skip to content

Modernize import hook #1369

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
filmor merged 21 commits into from
Jun 18, 2021
Merged

Modernize import hook #1369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a321daa
(WIP) modernize the import hook
BadSingleton Jan 15, 2021
279b535
Add the loaded namespaces tracking
BadSingleton Jan 15, 2021
f92e95b
cleanup and changelog entry
BadSingleton Jan 15, 2021
afffc18
Fix a bug where clr wasn't in sys.modules after reload
BadSingleton Jan 18, 2021
d821c0f
Further refinements to setattr logic on ModuleObjects
BadSingleton Jan 19, 2021
e469a8a
fixups, add docs
BadSingleton Jan 20, 2021
685b972
merge fixup
BadSingleton Mar 4, 2021
be81364
(WIP) import hook in the pytohn module
BadSingleton Mar 10, 2021
73958ed
Revert "(WIP) import hook in the pytohn module"
BadSingleton Apr 12, 2021
e71a0ef
Import hook as a module, take 2
BadSingleton Apr 12, 2021
2af066d
fixup! Merge remote-tracking branch 'origin/master' into modernize-im…
BadSingleton Apr 12, 2021
bb490bf
fixup! fixup! Merge remote-tracking branch 'origin/master' into moder…
BadSingleton Apr 12, 2021
31ea876
Create a clr.loader module
BadSingleton Jun 1, 2021
c02d5c6
Test to test if the test passes
BadSingleton Jun 2, 2021
970a189
fix new exception usage
BadSingleton Jun 2, 2021
059605b
kick the build because I can't repro
BadSingleton Jun 2, 2021
ff170e9
Add Namespaces to the import hook only through AddReference
BadSingleton Jun 2, 2021
63de923
Review changes, update API usage
BadSingleton Jun 8, 2021
624f7e3
Merge remote-tracking branch 'upstream/master' into modernize-import-…
BadSingleton Jun 14, 2021
bd7e745
make PyModule_AddObject in line with CPython
BadSingleton Jun 14, 2021
46a85fe
take care of stragglers
BadSingleton Jun 15, 2021
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
Prev Previous commit
Next Next commit
Review changes, update API usage
  • Loading branch information
@BadSingleton
BadSingleton committed Jun 8, 2021
commit 63de923a44259732525810bcbcca93bb963d0b9d
2 changes: 1 addition & 1 deletion src/embed_tests/pyimport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public void SetUp()
Assert.IsFalse(str == IntPtr.Zero);
BorrowedReference path = Runtime.Runtime.PySys_GetObject("path");
Assert.IsFalse(path.IsNull);
Runtime.Runtime.PyList_Append(path, str);
Runtime.Runtime.PyList_Append(path, new BorrowedReference(str));
Runtime.Runtime.XDecref(str);
}

Expand Down
1 change: 0 additions & 1 deletion src/runtime/BorrowedReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ readonly ref struct BorrowedReference
{
readonly IntPtr pointer;
public bool IsNull => this.pointer == IntPtr.Zero;
public bool IsNone => this.pointer == Runtime.PyNone;

/// <summary>Gets a raw pointer to the Python object</summary>
public IntPtr DangerousGetAddress()
Expand Down
69 changes: 24 additions & 45 deletions src/runtime/importhook.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ internal static void RestoreRuntimeData(RuntimeDataStorage storage)
var rootHandle = storage.GetValue<IntPtr>("root");
root = (CLRModule)ManagedType.GetManagedObject(rootHandle);
BorrowedReference dict = Runtime.PyImport_GetModuleDict();
Runtime.PyDict_SetItemString(dict.DangerousGetAddress(), "clr", py_clr_module);
Runtime.PyDict_SetItemString(dict, "clr", ClrModuleReference);
SetupNamespaceTracking();
}

Expand All @@ -115,14 +115,12 @@ static void SetupImportHook()
var exec = Runtime.PyDict_GetItemString(builtins, "exec");
using var args = NewReference.DangerousFromPointer(Runtime.PyTuple_New(2));

IntPtr codeStr = Runtime.PyString_FromString(LoaderCode);
Runtime.PyTuple_SetItem(args.DangerousGetAddress(), 0, codeStr);
// PyTuple_SetItem steals a reference, mod_dict is borrowed.
var codeStr = NewReference.DangerousFromPointer(Runtime.PyString_FromString(LoaderCode));
Runtime.PyTuple_SetItem(args, 0, codeStr);
var mod_dict = Runtime.PyModule_GetDict(import_hook_module);
Runtime.XIncref(mod_dict.DangerousGetAddress());
Runtime.PyTuple_SetItem(args.DangerousGetAddress(), 1, mod_dict.DangerousGetAddress());
Runtime.PyObject_Call(exec.DangerousGetAddress(), args.DangerousGetAddress(), IntPtr.Zero);

// reference not stolen due to overload incref'ing for us.
Runtime.PyTuple_SetItem(args, 1, mod_dict);
Runtime.PyObject_Call(exec, args, default);
// Set as a sub-module of clr.
if(Runtime.PyModule_AddObject(ClrModuleReference, "loader", import_hook_module) != 0)
{
Expand All @@ -132,9 +130,8 @@ static void SetupImportHook()

// Finally, add the hook to the meta path
var findercls = Runtime.PyDict_GetItemString(mod_dict, "DotNetFinder");
var finderCtorArgs = Runtime.PyTuple_New(0);
var finder_inst = Runtime.PyObject_CallObject(findercls.DangerousGetAddress(), finderCtorArgs);
Runtime.XDecref(finderCtorArgs);
var finderCtorArgs = NewReference.DangerousFromPointer(Runtime.PyTuple_New(0));
var finder_inst = Runtime.PyObject_CallObject(findercls, finderCtorArgs);
var metapath = Runtime.PySys_GetObject("meta_path");
Runtime.PyList_Append(metapath, finder_inst);
}
Expand All @@ -147,34 +144,19 @@ static void SetupImportHook()
/// </summary>
static void SetupNamespaceTracking()
{
var newset = Runtime.PySet_New(default);
try
using var newset = Runtime.PySet_New(default);
foreach (var ns in AssemblyManager.GetNamespaces())
{
foreach (var ns in AssemblyManager.GetNamespaces())
using var pyNs = NewReference.DangerousFromPointer(Runtime.PyString_FromString(ns));
if (Runtime.PySet_Add(newset, pyNs) != 0)
{
var pyNs = Runtime.PyString_FromString(ns);
try
{
if (Runtime.PySet_Add(newset, new BorrowedReference(pyNs)) != 0)
{
throw PythonException.ThrowLastAsClrException();
}
}
finally
{
Runtime.XDecref(pyNs);
}
throw PythonException.ThrowLastAsClrException();
}

if (Runtime.PyDict_SetItemString(root.dict, availableNsKey, newset.DangerousGetAddress()) != 0)
if (Runtime.PyDict_SetItemString(root.DictRef, availableNsKey, newset) != 0)
{
throw PythonException.ThrowLastAsClrException();
}
}
finally
{
newset.Dispose();
}

}

Expand All @@ -189,24 +171,21 @@ static void TeardownNameSpaceTracking()

public static void AddNamespace(string name)
{
using (Py.GIL())
var pyNs = Runtime.PyString_FromString(name);
try
{
var pyNs = Runtime.PyString_FromString(name);
try
var nsSet = Runtime.PyDict_GetItemString(new BorrowedReference(root.dict), availableNsKey);
if (!(nsSet.IsNull || nsSet.DangerousGetAddress() == Runtime.PyNone))
{
var nsSet = Runtime.PyDict_GetItemString(new BorrowedReference(root.dict), availableNsKey);
if (!(nsSet.IsNull && nsSet.IsNone))
if (Runtime.PySet_Add(nsSet, new BorrowedReference(pyNs)) != 0)
{
if (Runtime.PySet_Add(nsSet, new BorrowedReference(pyNs)) != 0)
{
throw PythonException.ThrowLastAsClrException();
}
throw PythonException.ThrowLastAsClrException();
}
}
finally
{
Runtime.XDecref(pyNs);
}
}
finally
{
Runtime.XDecref(pyNs);
}
}

Expand Down
8 changes: 3 additions & 5 deletions src/runtime/moduleobject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public void LoadNames()
var pyname = Runtime.PyString_FromString(name);
try
{
if (Runtime.PyList_Append(new BorrowedReference(__all__), pyname) != 0)
if (Runtime.PyList_Append(new BorrowedReference(__all__), new BorrowedReference(pyname)) != 0)
{
throw PythonException.ThrowLastAsClrException();
}
Expand Down Expand Up @@ -598,10 +598,8 @@ public static string[] ListAssemblies(bool verbose)
public static ModuleObject _load_clr_module(PyObject spec)
{
ModuleObject mod = null;
using (var modname = spec.GetAttr("name"))
{
mod = ImportHook.Import(modname.ToString());
}
using var modname = spec.GetAttr("name");
mod = ImportHook.Import(modname.ToString());
return mod;
}
Comment on lines +600 to +604
Copy link
Copy Markdown
Member

@lostmsu lostmsu Jun 3, 2021

Choose a reason for hiding this comment

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

NIT:

using var  modname = ...;
return ...;

}
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/pylist.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public static PyList AsList(PyObject value)
/// </remarks>
public void Append(PyObject item)
{
int r = Runtime.PyList_Append(this.Reference, item.obj);
int r = Runtime.PyList_Append(this.Reference, new BorrowedReference(item.obj));
if (r < 0)
{
throw PythonException.ThrowLastAsClrException();
Expand Down
26 changes: 22 additions & 4 deletions src/runtime/runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ internal static void Initialize(bool initSigs = false, ShutdownMode mode = Shutd
IntPtr item = PyString_FromString(rtdir);
if (PySequence_Contains(path, item) == 0)
{
PyList_Append(new BorrowedReference(path), item);
PyList_Append(new BorrowedReference(path), new BorrowedReference(item));
}
XDecref(item);
AssemblyManager.UpdatePath();
Expand Down Expand Up @@ -1087,6 +1087,8 @@ internal static IntPtr PyObject_GetAttr(IntPtr pointer, IntPtr name)


internal static IntPtr PyObject_Call(IntPtr pointer, IntPtr args, IntPtr kw) => Delegates.PyObject_Call(pointer, args, kw);
internal static IntPtr PyObject_Call(BorrowedReference pointer, BorrowedReference args, BorrowedReference kw)
=> Delegates.PyObject_Call(pointer.DangerousGetAddress(), args.DangerousGetAddress(), kw.DangerousGetAddressOrNull());


internal static NewReference PyObject_CallObject(BorrowedReference callable, BorrowedReference args) => Delegates.PyObject_CallObject(callable, args);
Expand Down Expand Up @@ -1822,7 +1824,7 @@ internal static int PyList_Insert(BorrowedReference pointer, long index, IntPtr
private static int PyList_Insert(BorrowedReference pointer, IntPtr index, IntPtr value) => Delegates.PyList_Insert(pointer, index, value);


internal static int PyList_Append(BorrowedReference pointer, IntPtr value) => Delegates.PyList_Append(pointer, value);
internal static int PyList_Append(BorrowedReference pointer, BorrowedReference value) => Delegates.PyList_Append(pointer, value);


internal static int PyList_Reverse(BorrowedReference pointer) => Delegates.PyList_Reverse(pointer);
Expand Down Expand Up @@ -1885,7 +1887,15 @@ internal static int PyTuple_SetItem(IntPtr pointer, long index, IntPtr value)
{
return PyTuple_SetItem(pointer, new IntPtr(index), value);
}
internal static int PyTuple_SetItem(BorrowedReference pointer, long index, StolenReference value)
=> PyTuple_SetItem(pointer.DangerousGetAddress(), new IntPtr(index), value.DangerousGetAddressOrNull());

internal static int PyTuple_SetItem(BorrowedReference pointer, long index, BorrowedReference value)
{
var increfValue = value.DangerousGetAddress();
Runtime.XIncref(increfValue);
return PyTuple_SetItem(pointer.DangerousGetAddress(), new IntPtr(index), increfValue);
}

private static int PyTuple_SetItem(IntPtr pointer, IntPtr index, IntPtr value) => Delegates.PyTuple_SetItem(pointer, index, value);

Expand Down Expand Up @@ -1944,6 +1954,14 @@ internal static string PyModule_GetFilename(IntPtr module)


internal static IntPtr PyImport_Import(IntPtr name) => Delegates.PyImport_Import(name);

/// <summary>
/// We can't use a StolenReference here because the reference is stolen only on success.
/// </summary>
/// <param name="module">The module to add the object to.</param>
/// <param name="name">The key that will refer to the object.</param>
/// <param name="stolenObject">The object to add to the module.</param>
/// <returns>Return -1 on error, 0 on success.</returns>
internal static int PyModule_AddObject(BorrowedReference module, string name, BorrowedReference stolenObject)
Copy link
Copy Markdown
Member

@lostmsu lostmsu Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like stolenObject parameter should be of type StolenReference here and below (present in the latest master).

Copy link
Copy Markdown
Contributor Author

@BadSingleton BadSingleton Jun 8, 2021

Choose a reason for hiding this comment

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

StolenReference here and below (present in the latest master)

🎉

Copy link
Copy Markdown
Contributor Author

@BadSingleton BadSingleton Jun 8, 2021

Choose a reason for hiding this comment

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

Although I'm reviewing the semantics, and this function only steals on success, so by using StolenReference, we leak on failure.

Copy link
Copy Markdown
Member

@lostmsu lostmsu Jun 9, 2021

Choose a reason for hiding this comment

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

Dang you are right. Why did they not make it consistent? PyTuple_SetItem for example does steal even when there's an error. ugh...

Maybe we should make a wrapper, that always steals?

Copy link
Copy Markdown
Member

@lostmsu lostmsu Jun 10, 2021

Choose a reason for hiding this comment

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

I think that we should not use BorrowedReference for the last parameter here. It makes false suggestion, that signature is valid, and borrowed reference is always enough when it is really not. I'd keep it IntPtr for now, and explain the behavior in the parameter doc.

Or alternatively, make a wrapper, that takes either BorrowedReference or StolenReference and handles refcounting internally, and keep the real PInvoke private.

Copy link
Copy Markdown
Contributor Author

@BadSingleton BadSingleton Jun 14, 2021

Choose a reason for hiding this comment

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

I'm leaning towards the IntPtr parameter to be as "consistant" as possible with the CPython API.

{
using var namePtr = new StrPtr(name, Encoding.UTF8);
Expand Down Expand Up @@ -2483,7 +2501,7 @@ static Delegates()
PyList_GetItem = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, BorrowedReference>)GetFunctionByName(nameof(PyList_GetItem), GetUnmanagedDll(_PythonDll));
PyList_SetItem = (delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, int>)GetFunctionByName(nameof(PyList_SetItem), GetUnmanagedDll(_PythonDll));
PyList_Insert = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, IntPtr, int>)GetFunctionByName(nameof(PyList_Insert), GetUnmanagedDll(_PythonDll));
PyList_Append = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int>)GetFunctionByName(nameof(PyList_Append), GetUnmanagedDll(_PythonDll));
PyList_Append = (delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, int>)GetFunctionByName(nameof(PyList_Append), GetUnmanagedDll(_PythonDll));
PyList_Reverse = (delegate* unmanaged[Cdecl]<BorrowedReference, int>)GetFunctionByName(nameof(PyList_Reverse), GetUnmanagedDll(_PythonDll));
PyList_Sort = (delegate* unmanaged[Cdecl]<BorrowedReference, int>)GetFunctionByName(nameof(PyList_Sort), GetUnmanagedDll(_PythonDll));
PyList_GetSlice = (delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, IntPtr>)GetFunctionByName(nameof(PyList_GetSlice), GetUnmanagedDll(_PythonDll));
Expand Down Expand Up @@ -2780,7 +2798,7 @@ static Delegates()
internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, BorrowedReference> PyList_GetItem { get; }
internal static delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, int> PyList_SetItem { get; }
internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, IntPtr, int> PyList_Insert { get; }
internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int> PyList_Append { get; }
internal static delegate* unmanaged[Cdecl]<BorrowedReference, BorrowedReference, int> PyList_Append { get; }
internal static delegate* unmanaged[Cdecl]<BorrowedReference, int> PyList_Reverse { get; }
internal static delegate* unmanaged[Cdecl]<BorrowedReference, int> PyList_Sort { get; }
internal static delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, IntPtr> PyList_GetSlice { get; }
Expand Down