[RuntimeAsync] Fix support for pinvokes#2847
[RuntimeAsync] Fix support for pinvokes#2847jakobbotsch merged 1 commit intodotnet:feature/async2-experimentfrom
Conversation
- Make sure async transformation leaves a scratch BB around for the pinvoke prolog to be inserted into - Make sure we properly insert pinvoke epilogs for `GT_RETURN_SUSPEND` nodes (needed on x86) - Ensure the pinvoke frame related locals are not captured by the async transformation
|
PTAL @VSadov |
| private static unsafe delegate* unmanaged<int> GetFPtr() => &GetValue; | ||
|
|
||
| [UnmanagedCallersOnly] | ||
| private static int GetValue() => 5; |
There was a problem hiding this comment.
A thought: what should happen if UnmanagedCallersOnly is applied to runtime async method.
Does it work today with ordinary async? (I guess, why not, they are just Task-returning methods)
There was a problem hiding this comment.
I haven't checked, but I imagine UnmanagedCallersOnly on any function returning an object reference should result in IlegalProgramException. So I would hope that was the case for async1, and we should similarly disallow it for async2.
There was a problem hiding this comment.
I haven't checked, but I imagine UnmanagedCallersOnly on any function returning an object reference should result in IlegalProgramException. So I would hope that was the case for async1, and we should similarly disallow it for async2.
I checked.
Even though the documentation only mentions that parameters must be blittable, the runtime requires that the return type is blittable as well. That rules out runtime async signatures. Even nongeneric ValueType, although a struct, is not blittable.
| { | ||
| // If previous first BB was a scratch BB then we have to recreate it | ||
| // after since later phases may be relying on it. | ||
| // TODO-Cleanup: The later phases should be creating it if they need it. |
There was a problem hiding this comment.
Do you plan to address this TODO or this is just a "good to have" comment?
There was a problem hiding this comment.
I do agree that relying on the presence of scratch is an arbitrary dependency that just happen to hold.
If creating on demand is not too hard, it would be better.
There was a problem hiding this comment.
Do you plan to address this TODO or this is just a "good to have" comment?
I don't plan to address it here, this should rather be cleaned up upstream.
I do agree that relying on the presence of scratch is an arbitrary dependency that just happen to hold.
If creating on demand is not too hard, it would be better.
The frontend does ensure it:
runtimelab/src/coreclr/jit/flowgraph.cpp
Lines 2315 to 2322 in 4daa761
It's simply that this is a very long-term invariant that is error prone and which feels unnecessary to me.
There was a problem hiding this comment.
Exactly. Such dependencies have negative value by default. They must be justified by advantages or scenarios that they enable. If arranging a scratch block was difficult to do later, it could be a possible justification, but, as I understand, it is not the case.
2 participants
GT_RETURN_SUSPENDnodes (needed on x86)