- Notifications
You must be signed in to change notification settings - Fork750
Proposal: Safe pointers#1043
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -292,7 +292,7 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth | |||
| for (int i = 0; i < pynkwargs; ++i) | |||
| { | |||
| var keyStr = Runtime.GetManagedString(Runtime.PyList_GetItem(keylist, i)); | |||
| kwargDict[keyStr] = Runtime.PyList_GetItem(valueList, i); | |||
| kwargDict[keyStr] = Runtime.PyList_GetItem(valueList, i).DangerousGetAddress(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This generates a compiler warning, that uncovers a potentially dangerous pattern, wherekwargsDict may contain borrowed references.
src/runtime/pydict.cs Outdated
| @@ -139,12 +139,13 @@ public PyObject Values() | |||
| /// </remarks> | |||
| public PyObject Items() | |||
| { | |||
| IntPtr items = Runtime.PyDict_Items(obj); | |||
| if (items == IntPtr.Zero) | |||
| using var items = Runtime.PyDict_Items(this.obj); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Afterdotnet/roslyn-analyzers#3305 is fixed we can configure build to fail here ifusing would be omitted
@amos402 can you check this one out too? |
codecov-io commentedFeb 14, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #1043 +/- ##==========================================+ Coverage 83.11% 86.75% +3.64%========================================== Files 1 1 Lines 302 302 ==========================================+ Hits 251 262 +11+ Misses 51 40 -11
Continue to review full report at Codecov.
|
You can also implement an implicit operator for PyObject make it easy for usage. |
@filmor I'd love if we get this in and take as new standard for coding Python handle manipulations, as it would make otherwise hard-to-debug segmentation fault/access violation bugs easier to prevent/spot/debug. |
@amos402 I can proceed to merge it, if you can sign-off a review, and are for this change in general. I do not plan to extend the scope yet (like making |
| @@ -83,6 +83,7 @@ | |||
| </Compile> | |||
| <Compile Include="arrayobject.cs" /> | |||
| <Compile Include="assemblymanager.cs" /> | |||
| <Compile Include="BorrowedReference.cs" /> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it good for separate these three items to different files? They're small, related, and it make the project files be scattered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The might get more code later on. Generally, I prefer a type per file approach for anything except delegate types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or create a folder for them? Just just too far if they sorted by name I thought🤣
| { | ||
| using System; | ||
| [NonCopyable] | ||
| ref struct NewReference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it necessary to limit it as a stack-allocated value? If make it to a normal struct and rename it toPyReference, it can be assigned to a collection and implement the IDisposable forusing () usage.
For better usage, aBorrowedReference may be able to promote to aPyReference for using the implict operate overloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With[NoCopyable] you won't be able to put it into a collection anyway. And without[NoCopyable] it does not provide any safety guarantees.
Having it asref struct saves you from addingref everywhere you take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But....sometimes I just want to put them into collections...😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then you can still doDangerousGetHandle or create aPyObject from them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But this type also has a purpose for reminding others that it's a reference don't forget to decref it, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because it must be clear from code, that care is necessary around this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, as mentioned above,[NoCopyable] would already prevent you from putting an instance into a collection, even ifref were removed. And[NoCopyable] is basically the sole purpose of this change, as if you do
NewReferencerefA=GetNewReferenceSomehow();NewReferencerefB=refA;
Then you already miappropriated refcounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe more introduce a type namedPyHandle for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
emmm, but that makeBorrowedReference embarrassed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@amos402 The idea withPyHandle can be reviewed separately. This PR is only for tracking new vs borrowed references as used in Python documentation for C API.
src/runtime/NewReference.cs Outdated
| public IntPtr Pointer { get; set; } | ||
| public bool IsNull => this.Pointer == IntPtr.Zero; | ||
| public PyObject ToPyObject() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I prefer two methods:ToPyObject: inref and create aPyObject, not assign thePointer to nullptrAsPyObject: use this implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Constructing a newPyObject from references should be done usingPyObject constructors (which can be added later). I agree though, thatToPyObject is not descriptive enough. MaybeMoveToPyObject orIntoPyObject?
| } | ||
| [Obsolete("Use overloads, that take BorrowedReference or NewReference")] | ||
| public IntPtr DangerousGetAddress() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Use a property namedAddress can be more fit?
Why I have to be warned if I trying use the raw pointer, seems it doesn't make any sense. Or just don't expose the interface and useexplicit operator IntPtr instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is modeled afterSafeHandle class and itsDangerousGetHandle method.
The whole point of introducing these references is to get rid ofIntPtrs entirely. It should warn you against converting intoIntPtr. MaybeObsolete part is unnecessary asDangerous sounds like a warning enough. I will removeObsolete.
removed public property Pointer from NewReference and replaced with DangerousGetAddress
lostmsu commentedFeb 26, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@amos402@filmor I urge you to review this ASAP. As I am investigating segfaults in#1050 and#1060 , I'd love to have type checker on my side. Will definitely bring this up during the next call. BTW, I don't think it is necessary to update branch with the latest master every time before merging, as long as there are no conflicts. In an unlikely case, when it breaks the build, the change can be reverted and the PR reopened. |
I like this a lot, I'm was just a bit reluctant about using C# 8 as it won't be officially supported on .NET Framework, but apparently you are not doing that yet. |
* NewReference type and an example usage* BorrowedReference + example, that exposes dangerous pattern* make BorrowedReference readonly ref struct* BorrowedReference.Pointer is a private readonly field* renamed NewReference.ToPyObject to MoveToPyObject* removed public property Pointer from NewReference and replaced with DangerousGetAddress
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
With C# 7.3
ref structsand C# 8.0usingworking on any method namedDisposeit is possible to track new vs borrowed references at compile time.This proposal introduces
BorrowedReferenceandNewReferencetypes, that areref-only structs, meaning you can't put them intoPyObjectdirectly, so they can implement differentToPyObjectmethods, one doingIncRef, and the other one not (alternatively, we can havePyObjectconstructor overloads). Along with Roslyn-basedNonCopyableAnalyzer andFxCop afterdotnet/roslyn-analyzers#3305 is fixed, this can ensure, that we do reference counting correctly at compile time.We can gradually change return and parameter types in PInvoke methods in
Runtimeto one ofBorrowedReferenceorNewReferenceto take advantage of this.Downsides
Build system must support C# 8.0, which is currently not supported in all build configurations.Without C# 8.0 one would have to dispose
NewReferenceinstances withtry ... finally ...block.