Skip to content

Commit 8baad6a

Browse files
author
Félix Bourbonnais
committed
Code review fixes
1 parent e7effd8 commit 8baad6a

File tree

2 files changed

+93
-105
lines changed

2 files changed

+93
-105
lines changed

‎src/runtime/metatype.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ public static IntPtr Initialize()
2424

2525
public static void Release()
2626
{
27+
if (PyCLRMetaType == IntPtr.Zero)
28+
{
29+
throw new ObjectDisposedException("PyCLRMetaType");
30+
}
2731
if (Runtime.Refcount(PyCLRMetaType) > 1)
2832
{
2933
_metaSlotsHodler.ResetSlots();

‎src/runtime/typemanager.cs

Lines changed: 89 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,8 @@ internal static IntPtr CreateType(ManagedType impl, Type clrType)
188188
if (typeof(ICollection).IsAssignableFrom(clrType) || clrType.GetInterfaces().Any(x => x.IsGenericType && x.GetGenericTypeDefinition() == typeof(ICollection<>)))
189189
{
190190
var method = typeof(mp_length_slot).GetMethod(nameof(mp_length_slot.mp_length));
191-
InitializeSlot(type, TypeOffset.mp_length, method, slotsHolder);
191+
var thunk = Interop.GetThunk(method);
192+
InitializeSlot(type, thunk, "__len__", slotsHolder);
192193
}
193194

194195
if (base_ != IntPtr.Zero)
@@ -354,6 +355,28 @@ internal static void FreeMethodDef(IntPtr mdef)
354355
}
355356
}
356357

358+
/// <summary>
359+
/// Adds a deallocator for a type's method. At deallocation, the deallocator will remove the
360+
/// method from the type's Dict and deallocate the PyMethodDef object.
361+
/// </summary>
362+
/// <param name="t">The type to add the deallocator to.</param>
363+
/// <param name="mdef">The pointer to the PyMethodDef structure.</param>
364+
/// <param name="name">The name of the slot.</param>
365+
/// <param name="slotsHolder">The SlotsHolder holding the deallocator/.</param>
366+
internal static void AddDeallocator(IntPtr t, IntPtr mdef, string name, SlotsHolder slotsHolder)
367+
{
368+
slotsHolder.AddDealloctor(() =>
369+
{
370+
//IntPtr t = type;
371+
IntPtr tp_dict = Marshal.ReadIntPtr(t, TypeOffset.tp_dict);
372+
if (Runtime.PyDict_DelItemString(tp_dict, name) != 0)
373+
{
374+
Runtime.PyErr_Print();
375+
}
376+
FreeMethodDef(mdef);
377+
});
378+
}
379+
357380
internal static IntPtr CreateMetaType(Type impl, out SlotsHolder slotsHolder)
358381
{
359382
// The managed metatype is functionally little different than the
@@ -389,54 +412,31 @@ internal static IntPtr CreateMetaType(Type impl, out SlotsHolder slotsHolder)
389412
Debug.Assert(4 * IntPtr.Size == Marshal.SizeOf(typeof(PyMethodDef)));
390413
IntPtr mdefStart = mdef;
391414
ThunkInfo thunk = Interop.GetThunk(typeof(MetaType).GetMethod("__instancecheck__"), "BinaryFunc");
392-
slotsHolder.KeeapAlive(thunk.Target);
393-
394-
{
395-
IntPtr mdefAddr = mdef;
396-
slotsHolder.AddDealloctor(() =>
397-
{
398-
IntPtr t = type;
399-
IntPtr tp_dict = Marshal.ReadIntPtr(t, TypeOffset.tp_dict);
400-
if (Runtime.PyDict_DelItemString(tp_dict, "__instancecheck__") != 0)
401-
{
402-
Runtime.PyErr_Print();
403-
}
404-
FreeMethodDef(mdefAddr);
405-
});
406-
}
415+
slotsHolder.KeepAlive(thunk.Target);
416+
// Add deallocator before writing the method def, as after WriteMethodDef, mdef
417+
// will not have the same value.
418+
AddDeallocator(type, mdef, "__instancecheck__", slotsHolder);
407419
mdef = WriteMethodDef(
408420
mdef,
409421
"__instancecheck__",
410422
thunk.Address
411423
);
412424

413425
thunk = Interop.GetThunk(typeof(MetaType).GetMethod("__subclasscheck__"), "BinaryFunc");
414-
slotsHolder.KeeapAlive(thunk.Target);
415-
{
416-
IntPtr mdefAddr = mdef;
417-
slotsHolder.AddDealloctor(() =>
418-
{
419-
IntPtr t = type;
420-
IntPtr tp_dict = Marshal.ReadIntPtr(t, TypeOffset.tp_dict);
421-
if (Runtime.PyDict_DelItemString(tp_dict, "__subclasscheck__") != 0)
422-
{
423-
Runtime.PyErr_Print();
424-
}
425-
FreeMethodDef(mdefAddr);
426-
});
427-
}
426+
slotsHolder.KeepAlive(thunk.Target);
427+
AddDeallocator(type, mdef, "__subclasscheck__", slotsHolder);
428428

429429
mdef = WriteMethodDef(
430430
mdef,
431431
"__subclasscheck__",
432432
thunk.Address
433433
);
434434

435-
// FIXME: mdef is not used
435+
// Pad the last field with zeroes to terminate the array
436436
mdef = WriteMethodDefSentinel(mdef);
437437

438438
Marshal.WriteIntPtr(type, TypeOffset.tp_methods, mdefStart);
439-
slotsHolder.Add(TypeOffset.tp_methods, (t, offset) =>
439+
slotsHolder.Set(TypeOffset.tp_methods, (t, offset) =>
440440
{
441441
var p = Marshal.ReadIntPtr(t, offset);
442442
Runtime.PyMem_Free(p);
@@ -818,9 +818,8 @@ internal static void InitializeSlots(IntPtr type, Type impl, SlotsHolder slotsHo
818818
// These have to be defined, though, so by default we fill these with
819819
// static C# functions from this class.
820820

821-
// var ret0 = Interop.GetThunk(((Func<IntPtr, int>)Return0).Method).Address;
822-
// var ret1 = Interop.GetThunk(((Func<IntPtr, int>)Return1).Method).Address;
823-
821+
IntPtr ret0 = IntPtr.Zero;
822+
IntPtr ret1 = IntPtr.Zero;
824823
if (native != null)
825824
{
826825
// If we want to support domain reload, the C# implementation
@@ -829,35 +828,18 @@ internal static void InitializeSlots(IntPtr type, Type impl, SlotsHolder slotsHo
829828
// load them into a separate code page that is leaked
830829
// intentionally.
831830
InitializeNativeCodePage();
832-
IntPtr ret1 = NativeCodePage + native.Return1;
833-
IntPtr ret0 = NativeCodePage + native.Return0;
834-
InitializeSlot(type, ret0, "tp_traverse", canOverride: false);
835-
InitializeSlot(type, ret0, "tp_clear", canOverride: false);
836-
InitializeSlot(type, ret1, "tp_is_gc", canOverride: false);
831+
ret1 = NativeCodePage + native.Return1;
832+
ret0 = NativeCodePage + native.Return0;
837833
}
838834
else
839835
{
840-
if (!IsSlotSet(type, "tp_traverse"))
841-
{
842-
var thunkRet0 = Interop.GetThunk(((Func<IntPtr, int>)Return0).Method);
843-
var offset = GetSlotOffset("tp_traverse");
844-
Marshal.WriteIntPtr(type, offset, thunkRet0.Address);
845-
if (slotsHolder != null)
846-
{
847-
slotsHolder.Add(offset, thunkRet0);
848-
}
849-
}
850-
if (!IsSlotSet(type, "tp_clear"))
851-
{
852-
var thunkRet0 = Interop.GetThunk(((Func<IntPtr, int>)Return0).Method);
853-
var offset = GetSlotOffset("tp_clear");
854-
Marshal.WriteIntPtr(type, offset, thunkRet0.Address);
855-
if (slotsHolder != null)
856-
{
857-
slotsHolder.Add(offset, thunkRet0);
858-
}
859-
}
836+
ret1 = Interop.GetThunk(((Func<IntPtr, int>)Return1).Method).Address;
837+
ret0 = Interop.GetThunk(((Func<IntPtr, int>)Return0).Method).Address;
860838
}
839+
840+
InitializeSlot(type, ret0, "tp_traverse");
841+
InitializeSlot(type, ret0, "tp_clear");
842+
InitializeSlot(type, ret1, "tp_is_gc");
861843
}
862844

863845
static int Return1(IntPtr _) => 1;
@@ -873,11 +855,11 @@ internal static void InitializeSlots(IntPtr type, Type impl, SlotsHolder slotsHo
873855
/// </summary>
874856
/// <param name="type">Type being initialized.</param>
875857
/// <param name="slot">Function pointer.</param>
876-
/// <param name="canOverride">Can override the slot when it existed</param>
877-
static void InitializeSlot(IntPtr type, IntPtr slot, string name, bool canOverride = true)
858+
/// <param name="name">Name of the slot to initialize</param>
859+
static void InitializeSlot(IntPtr type, IntPtr slot, string name)
878860
{
879861
var offset = GetSlotOffset(name);
880-
if (!canOverride && Marshal.ReadIntPtr(type, offset) != IntPtr.Zero)
862+
if (Marshal.ReadIntPtr(type, offset) != IntPtr.Zero)
881863
{
882864
return;
883865
}
@@ -901,16 +883,6 @@ static void InitializeSlot(IntPtr type, ThunkInfo thunk, string name, SlotsHolde
901883
}
902884
}
903885

904-
static void InitializeSlot(IntPtr type, int slotOffset, MethodInfo method, SlotsHolder slotsHolder = null)
905-
{
906-
var thunk = Interop.GetThunk(method);
907-
Marshal.WriteIntPtr(type, slotOffset, thunk.Address);
908-
if (slotsHolder != null)
909-
{
910-
slotsHolder.Add(slotOffset, thunk);
911-
}
912-
}
913-
914886
static int GetSlotOffset(string name)
915887
{
916888
Type typeOffset = typeof(TypeOffset);
@@ -981,88 +953,100 @@ private static SlotsHolder CreateSlotsHolder(IntPtr type)
981953

982954
class SlotsHolder
983955
{
984-
public delegate void Resetor(IntPtr type, int offset);
956+
/// <summary>
957+
/// Delegate called to customize a (Python) Type slot reset
958+
/// </summary>
959+
/// <param name="type">The type that will have a slot reset</param>
960+
/// <param name="offset">The offset of the slot</param>
961+
public delegate void ResetSlotAction(IntPtr type, int offset);
985962

986-
private readonly IntPtr _type;
987-
private Dictionary<int, ThunkInfo> _slots = new Dictionary<int, ThunkInfo>();
988-
private List<Delegate> _keepalive = new List<Delegate>();
989-
private Dictionary<int, Resetor> _customRestors = new Dictionary<int, Resetor>();
990-
private List<Action> _deallocators = new List<Action>();
991-
private bool _alredyReset = false;
963+
private readonly IntPtr type;
964+
private Dictionary<int, ThunkInfo> slots = new Dictionary<int, ThunkInfo>();
965+
private List<Delegate> keepalive = new List<Delegate>();
966+
private Dictionary<int, ResetSlotAction> customResetors = new Dictionary<int, ResetSlotAction>();
967+
private List<Action> deallocators = new List<Action>();
968+
private bool alreadyReset = false;
992969

993970
/// <summary>
994971
/// Create slots holder for holding the delegate of slots and be able to reset them.
995972
/// </summary>
996973
/// <param name="type">Steals a reference to target type</param>
997974
public SlotsHolder(IntPtr type)
998975
{
999-
_type = type;
976+
this.type = type;
1000977
}
1001978

1002979
public void Add(int offset, ThunkInfo thunk)
1003980
{
1004-
_slots.Add(offset, thunk);
981+
slots.Add(offset, thunk);
1005982
}
1006983

1007-
public void Add(int offset, Resetor resetor)
984+
public void Set(int offset, ResetSlotAction resetor)
1008985
{
1009-
_customRestors[offset] = resetor;
986+
customResetors[offset] = resetor;
1010987
}
1011988

1012989
public void AddDealloctor(Action deallocate)
1013990
{
1014-
_deallocators.Add(deallocate);
991+
deallocators.Add(deallocate);
1015992
}
1016993

1017-
public void KeeapAlive(Delegate d)
994+
/// <summary>
995+
/// Add a delegate to keep it from being garbage collected.
996+
/// </summary>
997+
/// <param name="d">The delegate to add</param>
998+
public void KeepAlive(Delegate d)
1018999
{
1019-
_keepalive.Add(d);
1000+
keepalive.Add(d);
10201001
}
10211002

10221003
public void ResetSlots()
10231004
{
1024-
if (_alredyReset)
1005+
if (alreadyReset)
10251006
{
10261007
return;
10271008
}
1028-
_alredyReset = true;
1029-
IntPtr tp_name = Marshal.ReadIntPtr(_type, TypeOffset.tp_name);
1030-
string typeName = Marshal.PtrToStringAnsi(tp_name);
1031-
foreach (var offset in _slots.Keys)
1009+
alreadyReset = true;
1010+
foreach (var offset in slots.Keys)
10321011
{
10331012
IntPtr ptr = GetDefaultSlot(offset);
10341013
//DebugUtil.Print($"Set slot<{TypeOffsetHelper.GetSlotNameByOffset(offset)}> to 0x{ptr.ToString("X")} at {typeName}<0x{_type}>");
1035-
Marshal.WriteIntPtr(_type, offset, ptr);
1014+
Marshal.WriteIntPtr(type, offset, ptr);
10361015
}
10371016

1038-
foreach (var action in _deallocators)
1017+
foreach (var action in deallocators)
10391018
{
10401019
action();
10411020
}
10421021

1043-
foreach (var pair in _customRestors)
1022+
foreach (var pair in customResetors)
10441023
{
10451024
int offset = pair.Key;
10461025
var resetor = pair.Value;
1047-
resetor?.Invoke(_type, offset);
1026+
resetor?.Invoke(type, offset);
10481027
}
10491028

1050-
_customRestors.Clear();
1051-
_slots.Clear();
1052-
_keepalive.Clear();
1053-
_deallocators.Clear();
1029+
customResetors.Clear();
1030+
slots.Clear();
1031+
keepalive.Clear();
1032+
deallocators.Clear();
10541033

10551034
// Custom reset
1056-
IntPtr tp_base = Marshal.ReadIntPtr(_type, TypeOffset.tp_base);
1035+
IntPtr tp_base = Marshal.ReadIntPtr(type, TypeOffset.tp_base);
10571036
Runtime.XDecref(tp_base);
1058-
Marshal.WriteIntPtr(_type, TypeOffset.tp_base, IntPtr.Zero);
1037+
Marshal.WriteIntPtr(type, TypeOffset.tp_base, IntPtr.Zero);
10591038

1060-
IntPtr tp_bases = Marshal.ReadIntPtr(_type, TypeOffset.tp_bases);
1039+
IntPtr tp_bases = Marshal.ReadIntPtr(type, TypeOffset.tp_bases);
10611040
Runtime.XDecref(tp_bases);
10621041
tp_bases = Runtime.PyTuple_New(0);
1063-
Marshal.WriteIntPtr(_type, TypeOffset.tp_bases, tp_bases);
1042+
Marshal.WriteIntPtr(type, TypeOffset.tp_bases, tp_bases);
10641043
}
10651044

1045+
/// <summary>
1046+
/// Returns the default C function pointer for the slot to reset.
1047+
/// </summary>
1048+
/// <param name="offset">The offset of the slot.</param>
1049+
/// <returns>The default C function pointer of the slot.</returns>
10661050
private static IntPtr GetDefaultSlot(int offset)
10671051
{
10681052
if (offset == TypeOffset.tp_clear
@@ -1072,7 +1056,7 @@ private static IntPtr GetDefaultSlot(int offset)
10721056
}
10731057
else if (offset == TypeOffset.tp_dealloc)
10741058
{
1075-
// tp_free of PyTypeType is point to PyObejct_GC_Del.
1059+
// tp_free of PyTypeType is point to PyObject_GC_Del.
10761060
return Marshal.ReadIntPtr(Runtime.PyTypeType, TypeOffset.tp_free);
10771061
}
10781062
else if (offset == TypeOffset.tp_free)

0 commit comments

Comments
 (0)