apply amos interop changes from soft shutdown by koubaa · Pull Request #1219 · pythonnet/pythonnet · GitHub

Changes taken from @amos402 soft shutdown branch

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Updated the CHANGELOG

@amos402 @koubaa
As far as I can see, this is not actually fixing or explaining anything. It just removes the assert, that caused the problem.

IMHO, the assert should stay. This PR does not address it, but probably solves the problem @koubaa has.

The checking of typeSize <= ExceptionOffset.Size() is totally a wrong idea, there's no connection between them. Why an other object size whould relates to an exception object size.
You can simply make it lager than that size by

class A(B, C, D, E, F, G): pass

@amos402 if I remember correctly, BaseOffset is only used for reflections of .NET classes created by Python.NET. Right now there are only two kinds of them: System.Exception and everything derived from it, that end up deriving Python's builtins.BaseException or builtins.Exception, and every other .NET type, which end up deriving from builtins.object. Within those two categories each of the types has identical structure, and the ones derived from Exception just happen to be larger.

When Python class is inherited from a reflected .NET class via single inheritance, and has no extra low-level PyType stuff defined explicitly, I see no reason for its objects to be larger, than corresponding base class objects. Hence all such types should have either the same size as reflected System.Object or as reflected System.Exception.

@lostmsu @amos402 if the check needs to be removed, it can be done in a follow-on PR. I don't think this discussion should hold this PR.

@koubaa I am fine with this PR without the check removal.

Codecov Report

Merging #1219 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1219   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          251      251           
  Misses         40       40

Flag	Coverage Δ
#setup_linux	64.94% <ø> (ø)
#setup_windows	72.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d44f1da...2db6fa0. Read the comment docs.

@amos402 @koubaa the fix is still incomplete. It places GCHandle past the last field of PyType (or Python class type), it never allocates space for it, hence it still writes to an offset it does not own.

For example, on Windows x64 Python 3.8 an instance of PyType is 880 bytes long. An instance of CLR Metatype, the type of all reflected classes, is also 880 bytes long. After this fix class Sub(System.Exception) invokes MetaType.tp_new, that has this line:

pythonnet/src/runtime/metatype.cs

Line 122 in 022be19

Marshal.WriteIntPtr(type, TypeOffset.magic(), gc);

Where it writes the GCHandle immediately past the last byte of the new type object (an instance of CLR Metatype), because TypeOffset.members is also 880.

To fix this it might be sufficient to increase tp_basicsize of CLR Metatype by IntPtr.Size to allocate space for GCHandle.

It allocated the space, you can check PyType_GenericAlloc. It has an extra space sizeof(PyMemberDef), as long you only use the first pointer which is points to the name and not to use wearef to CLR types, it's fine. If you want to use more space roughly, you should skip the memory of type, because the typeobject may check it.
If you want a more complete general solution, you can check this amos402#5

koubaa force-pushed the amos-interop-changes branch from ef44edd to 630da19 Compare September 13, 2020 02:45

koubaa changed the title apply amos changes from soft shutdown apply amos interop changes from soft shutdown Sep 13, 2020

lostmsu requested changes Sep 13, 2020

View reviewed changes

Comment thread src/runtime/interop.cs

lostmsu reviewed Sep 13, 2020

View reviewed changes

Comment thread src/runtime/interop.cs

lostmsu reviewed Sep 13, 2020

View reviewed changes

Comment thread src/runtime/interop.cs Outdated

lostmsu requested changes Sep 15, 2020

View reviewed changes

Comment thread setup.py Outdated

Comment thread src/runtime/clrobject.cs Outdated

Comment thread src/runtime/managedtype.cs

Comment thread pythonnet.15.sln Outdated

koubaa force-pushed the amos-interop-changes branch from fc8cdb7 to 5ef0adf Compare September 16, 2020 00:44

koubaa added 5 commits September 16, 2020 19:41

apply amos changes from soft shutdown

776ae32

fix other versions

e8a53d0

interop template

c26c6f8

fixes

bb1ebe3

use HasFlag

7f576cf

koubaa force-pushed the amos-interop-changes branch from 5ef0adf to 7f576cf Compare September 17, 2020 00:42

filmor requested a review from lostmsu September 17, 2020 15:37

lostmsu reviewed Sep 17, 2020

View reviewed changes

Comment thread src/runtime/interop.cs Outdated

koubaa and others added 2 commits September 17, 2020 20:52

remove unused dictionary

8e1fa20

Merge branch 'master' into amos-interop-changes

022be19

filmor added 2 commits October 1, 2020 14:51

Merge branch 'master' into amos-interop-changes

2db6fa0

Merge branch 'master' into amos-interop-changes

079150c

koubaa closed this Oct 10, 2020

amos402 mentioned this pull request Oct 22, 2020

Weakref support #1267

Closed

1 of 4 tasks

                           Debug.Assert(type != IntPtr.Zero);
                           int typeSize = Marshal.ReadInt32(type, TypeOffset.tp_basicsize);
-                          Debug.Assert(typeSize > 0 && typeSize <= ExceptionOffset.Size());
+                          Debug.Assert(typeSize > 0);

apply amos interop changes from soft shutdown by koubaa · Pull Request #1219 · pythonnet/pythonnet · GitHub

Repository navigation

apply amos interop changes from soft shutdown - #1219

apply amos interop changes from soft shutdown#1219koubaa wants to merge 9 commits intopythonnet:masterpythonnet/pythonnet:masterfrom koubaa:amos-interop-changeskoubaa/pythonnet:amos-interop-changesCopy head branch name to clipboard

Conversation

koubaa commented Sep 13, 2020

Uh oh!

lostmsu Sep 13, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amos402 Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

koubaa Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lostmsu commented Sep 22, 2020

Uh oh!

amos402 commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

apply amos interop changes from soft shutdown#1219
koubaa wants to merge 9 commits into
pythonnet:masterfrom
koubaa:amos-interop-changes

lostmsu Sep 13, 2020 •
edited
Loading

codecov-commenter commented Sep 14, 2020 •
edited
Loading

amos402 commented Sep 24, 2020 •
edited
Loading