- Notifications
You must be signed in to change notification settings - Fork748
Fix illegal delegate usage#1328
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
Fix illegal delegate usage#1328
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Cache the delegates to native code we've created so that when we need tocall that delegate, we don't ask for a delegate from a native pointer(that is a delegate to managed code.)
Test failures are the same as the ones in the current master:https://github.com/pythonnet/pythonnet/runs/1571830014 |
src/runtime/nativecall.cs Outdated
| if (!Interop.allocatedThunks.TryGetValue(fp, out d)) | ||
| { | ||
| // Use Marshal.GetDelegateForFunctionPointer<> directly after upgrade the framework | ||
| d = Marshal.GetDelegateForFunctionPointer(fp, typeof(T)); |
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.
Can you also apply the comment here? The generic function was introduced in .NET 4.5.1.
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.
Actually, maybe just remove the comment. You are not writing this intoallocatedThunks right now, btw.
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.
Yes, I don't cache these because they truly point to unmanaged code. We should only cache the function pointers to managed code.
| // We don't cache this delegate because this is a pure delegate ot unmanaged. | ||
| d = Marshal.GetDelegateForFunctionPointer<T>(fp); | ||
| } | ||
| return (T)d; |
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.
You'd save a couple instructions with:
Delegate d; // no initif (trygetvalue(out d)) { return (T)d;} else { return Marshal.GetDelegate...(); // no cast}| Delegate d = null; | ||
| if (!Interop.allocatedThunks.TryGetValue(fp, out d)) |
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.
NIT: you don't need to assign an initial value tod. You could also doTryGetValue(fp, out var d).
Uh oh!
There was an error while loading.Please reload this page.
What does this implement/fix? Explain your changes.
Cache the delegates to native code we've created so that when we need to
call that delegate, we don't ask for a delegate from a native pointer
(that is a delegate to managed code.)
Does this close any currently open issues?
This fixes issue#1323 .
Any other comments?
This is needed in order for#1287 to pass the domain reload tests on Mono.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG