Please, move the original comment here.

What comment?

NIT: You seem to have moved this fromCLRObject, and removed

// Fix the BaseException args (and __cause__ in case of Python 3)// slot if wrapping a CLR exception

The comment onSetArgsAndCause won't be enough?

This seem to be silently stealing a reference totarget. I'd prefer it to explicitly takeNewReference orref NewReference (to clear the original variable), or takeBorrowedReference andIncRef here.

Ideally, I would also checkob andoffset for sanity.

Also, this really should also be outside theRuntime class, but I am not sure where is best to put it.

The original version is the C macro, the implementation used pointer offset, maybe is not suited for such a high-level type to be involved. I want to keep such things simple, If you need that, maybe create a high-level wrapper.😓

NewReference is not high-level. It is just a struct.

Then you have to pull the pointer and add an offset, what a tortuous path.

It is better, than accidentally forgetting to incref thetarget in the caller.

Please, remove dead code entirely.

Can you also comment why is this no longer necessary?

These slots can inherit from the base type automatically. Emmm, I can add some comments here, but it still needs the dead code comment as an explanation.

The comment above also seems dangerously wrong.

Hmmm, that was an old comment, that time before we unify the shutdown progresses, it can't and won't use tp_traverse and tp_clear. Seems it can be removed at one time.

Any dead code should be removed immediately.

We should try to use*Reference types for clarity for all new PInvoke functions.

That makes sense, but can we just replace them all at once.😅

@amos402 are you prepared to make this happen? )

It is a very bad idea to use private function. Why is it required? Can the same be done with only public functions?

I'm afraid there's no such public function now, it's required to allocate a custom size Python GC object.

Specifically, can we usePyType_FromSpec here?

Maybe not,PyType_FromSpec still usePyType_Type to allocate memory, which means it would occur the same problem as before.

Actually, I think I solved this one. Please see9682dc6
So you should not need_PyObject_GC_Calloc anymore.

No, it didn't, seehttps://github.com/python/cpython/blob/fbc7723778be01b8f3bb72d2dcac15ab9fbb9923/Objects/typeobject.c#L1051-L1082

Yes, it did, seehttps://github.com/python/cpython/blob/8b3414818f5289eac530bf38bcfbd7b2b851805c/Include/cpython/objimpl.h#L21 :-P

It's not about the type size which the MetaType can generate. It's about the size of MetaType itself. You can't create the MetaType by usingPyType_GenericAlloc(PyTypeType, 0), unless you

old_size=PyTypeType.tp_basicsize;PyTypeType.tp_basicsize+=x;PyType_GenericAlloc(PyTypeType,0);PyTypeType.tp_basicsize=old_size;

As long as it claimed itself as a managed type, it should have a slot indicate the place of its instances where the related GCHandle would be stored. As for now, the MetaType doesn't attach any C# object, so it remains 0. Look back to the master, if you put the PyCLRMetaType toGetManagedObject, actually it would read the memory it should not read(the part of the reservedtp_itemsize). The code doesn't explicitly call something like that, but something may trigger it does, like domain reload may visit it through PyVisit. Even it won't crash right now, but it's hard to say that's a kind of sanity code.
Of course, you can just remove the Manged flag from PyCLRMetaType to avoid that. But I suggest following the old rule: the type of a CLR type is a managed type.
Another approach is to allocate with PyObject_Malloc, but that means it has to be initialized manually, e.g. call_Py_NewReference manually on debug version, add it to GCTrack.
Nevertheless, the point is, the type must clarify itself what's itself and what it can do, that commit avoided some problems, but didn't solve completely.

As far as I can see, CLR Metatype does not containGCHandle, so it does not needGetManagedObject to work on its instances. So I think my implementation linked above is correct. PerhapsPyCLRMetaType should not be a managed object then?

It indeed has a bit of struggle with that, the PyCLRMetaType it's the ancestor of all managed types, if it claimed it's not a managed type, it against the commonsense. Currently, it doesn't need to be bound with a clr instance even it can. Well, just an identity problem.

I think it makes sense, that CLR metatype isnot a managed type. In my mind managed type == type, whose implementation is managed == has a managed object, that implements functionality == hasGCHandle pointing to that object. CLR metatype does not have aGCHandle.

Why do you think this is better, than the single line you removed from ABI.cs? This is hacky and uses reflection.

It's not related to that line, the reason for removing that line just because was because that's not the fixed slot anymore.
Well, it was just need something to store the size, tp_basicsize might capable do that too, this part can be improved.

This field has the same purpose and value as the one you removed from ABI -sizeof(PyTypeObject)

NIT: use C# 9 using syntax

Any dead code should be removed immediately.

These two refer to different handles, yet are named similarly, which is confusing.

Besides, in C# 9 we can start usingnint instead ofIntPtr for non-pointers.

What name do you prefer?

This is just a NIT. MaybeTypeClrHandle andInstanceClrHandleOffset?

I feel likeExceptions.GetExceptionHandle should be built intoCLRObject.GetInstHandle.

The exception object is a special case, we should keep it outside from the generic caseGetInstHandle, for example, if it has multiple special cases instead of one, you won't want to contaminate the CLRObject methods, right? Just like I removed the special case code from the CLRObject's constructor.

Why can't you do this unconditionally? This condition makes reasoning about the state much harder.

Ifobj.RefCount == 1, the nextXDecref would dealloc the obj, thus it's no need to call these things.

Is this similar totp_clear in any way? I would suggest to extract these two updates (FreeGCHandle and zeroing out) into a method.

The point beingtp_clear has certain semantics, so it is easier to understand code containing a call to it, rather than has same actions inline.

Also, ifRefCount will correctly return 0, I'd move thisif below.

Not exactly,tp_clear is used to clear the Python object reference in order to break reference cycles. These codes are used to disconnect the relation between Python and clr, if the RefCount reached 0, their relation would naturally be gone.
Their relation needs to be kept if RefCount > 1 because intp_dealloc, it needs to obtain the bound object.
tp_dealloc didn't zero out the handle because this chunk of memory would be freed soon aftertp_dealloc, extract these into a method is not really necessary.
Since after dealloc the memory may be released, theif scope cannot move to below.

As far as I can see, CLR Metatype does not containGCHandle, so it does not needGetManagedObject to work on its instances. So I think my implementation linked above is correct. PerhapsPyCLRMetaType should not be a managed object then?

Is this similar totp_clear in any way? I would suggest to extract these two updates (FreeGCHandle and zeroing out) into a method.

The point beingtp_clear has certain semantics, so it is easier to understand code containing a call to it, rather than has same actions inline.

Also, ifRefCount will correctly return 0, I'd move thisif below.

I think it makes sense, that CLR metatype isnot a managed type. In my mind managed type == type, whose implementation is managed == has a managed object, that implements functionality == hasGCHandle pointing to that object. CLR metatype does not have aGCHandle.