Skip to content

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.

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

apply amos interop changes from soft shutdown #1219

wants to merge 9 commits into from

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Sep 13, 2020

Fixes #1218

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

@koubaa koubaa force-pushed the amos-interop-changes branch from ef44edd to 630da19 Compare September 13, 2020 02:45
@koubaa koubaa changed the title apply amos changes from soft shutdown apply amos interop changes from soft shutdown Sep 13, 2020
lostmsu
lostmsu requested changes Sep 13, 2020
View reviewed changes
src/runtime/interop.cs Outdated

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);
Copy link
Member

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

@lostmsu lostmsu Sep 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 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

Copy link
Member

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

Copy link
Contributor Author

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

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
lostmsu requested changes Sep 13, 2020
View reviewed changes
lostmsu
lostmsu reviewed Sep 13, 2020
View reviewed changes
lostmsu
lostmsu reviewed Sep 13, 2020
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

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

lostmsu
lostmsu requested changes Sep 15, 2020
View reviewed changes
@koubaa koubaa force-pushed the amos-interop-changes branch from fc8cdb7 to 5ef0adf Compare September 16, 2020 00:44
@koubaa koubaa force-pushed the amos-interop-changes branch from 5ef0adf to 7f576cf Compare September 17, 2020 00:42
@filmor filmor requested a review from lostmsu September 17, 2020 15:37
lostmsu
lostmsu reviewed Sep 17, 2020
View reviewed changes
@lostmsu
Copy link
Member

lostmsu commented Sep 22, 2020

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

@amos402
Copy link
Member

amos402 commented Sep 24, 2020 •
edited
Loading

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 koubaa closed this Oct 10, 2020
@amos402 amos402 mentioned this pull request Oct 22, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor filmor left review comments

@lostmsu lostmsu lostmsu requested changes

@amos402 amos402 amos402 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multiple assertion failures for Python classes derived from System.Exception in debug builds
5 participants
@koubaa @codecov-commenter @lostmsu @amos402 @filmor