NotificationsYou must be signed in to change notification settings
Fork750
Star5.1k

apply amos interop changes from soft shutdown#1219

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Closed

koubaa wants to merge9 commits intopythonnet:masterfromkoubaa:amos-interop-changes

Closed

apply amos interop changes from soft shutdown#1219

koubaa wants to merge9 commits intopythonnet:masterfromkoubaa:amos-interop-changes

Conversation

Copy link

Contributor

Choose a reason for hiding this comment

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

@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.

Copy link

Member

lostmsuSep 13, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

The checking oftypeSize <= 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

classA(B,C,D,E,F,G):pass

Copy link

Member

lostmsuSep 20, 2020

Choose a reason for hiding this comment

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

@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'sbuiltins.BaseException orbuiltins.Exception, and every other .NET type, which end up deriving frombuiltins.object. Within those two categories each of the types has identical structure, and the ones derived fromException 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 reflectedSystem.Object or as reflectedSystem.Exception.

Copy link

ContributorAuthor

koubaaSep 21, 2020

Choose a reason for hiding this comment

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

@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.

Copy link

Member

lostmsuSep 21, 2020

Choose a reason for hiding this comment

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

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

lostmsu requested changes

Sep 13, 2020

View reviewed changes

src/runtime/interop.csShow resolvedHide resolved

lostmsu reviewed

Sep 13, 2020

View reviewed changes

src/runtime/interop.cs OutdatedShow resolvedHide resolved

lostmsu reviewed

Sep 13, 2020

View reviewed changes

src/runtime/interop.cs OutdatedShow resolvedHide resolved

Copy link

codecov-commenter commentedSep 14, 2020•
edited
Loading

Codecov Report

Merging#1219 intomaster willnot change coverage.
The diff coverage isn/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 byCodecov. Last updated44f1da...2db6fa0. Read thecomment docs.

lostmsu requested changes

Sep 15, 2020

View reviewed changes

setup.py OutdatedShow resolvedHide resolved

src/runtime/clrobject.cs OutdatedShow resolvedHide resolved

src/runtime/managedtype.csShow resolvedHide resolved

pythonnet.15.sln OutdatedShow resolvedHide resolved

koubaa force-pushed theamos-interop-changes branch fromfc8cdb7 to5ef0adfCompare

September 16, 2020 00:44

koubaa added5 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 theamos-interop-changes branch from5ef0adf to7f576cfCompare

September 17, 2020 00:42

filmor requested a review fromlostmsu

September 17, 2020 15:37

lostmsu reviewed

Sep 17, 2020

View reviewed changes

src/runtime/interop.cs OutdatedShow resolvedHide resolved

koubaaand others added2 commits

September 17, 2020 20:52

remove unused dictionary

8e1fa20

Merge branch 'master' into amos-interop-changes

022be19

Copy link

Member

lostmsu commentedSep 22, 2020

@amos402 @koubaa the fix is still incomplete. It placesGCHandle past the last field ofPyType (or Python classtype), 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 ofCLR Metatype, the type of all reflected classes, is also 880 bytes long. After this fixclass Sub(System.Exception) invokesMetaType.tp_new, that has this line:

pythonnet/src/runtime/metatype.cs

Line 122 in022be19

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

Where it writes theGCHandle immediately past the last byte of the new type object (an instance ofCLR Metatype), becauseTypeOffset.members is also 880.

To fix this it might be sufficient to increasetp_basicsize ofCLR Metatype byIntPtr.Size to allocate space forGCHandle.

Copy link

Member

amos402 commentedSep 24, 2020•
edited
Loading

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

filmor added2 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

4 tasks

Labels

None yet

5 participants


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

Movatterモバイル変換

apply amos interop changes from soft shutdown#1219

apply amos interop changes from soft shutdown#1219

Uh oh!

Conversation

koubaa commentedSep 13, 2020

Uh oh!

lostmsuSep 13, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 13, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amos402Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 20, 2020

Choose a reason for hiding this comment

Uh oh!

koubaaSep 21, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commentedSep 14, 2020• editedLoading 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 commentedSep 22, 2020

Uh oh!

amos402 commentedSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lostmsuSep 13, 2020•
edited
Loading

codecov-commenter commentedSep 14, 2020•
edited
Loading

amos402 commentedSep 24, 2020•
edited
Loading