Avoid eliding nested pointer-to-ref conversions#82263
Conversation
|
Done with review pass (commit 2) #Closed |
hamarb123
left a comment
There was a problem hiding this comment.
lgtm other than the missing case I commented on.
There was a problem hiding this comment.
I find the codegen for the
ref byte b1 = ref F();
ref byte b2 = ref (b1 = ref *p);
b2.ToString();test to be quite suspicious (for the same reason the other previously problematic code samples were suspicious) and potentially problematic if it can still be like that with other calls added between w/o forcing it to go to the byref local.
hamarb123
left a comment
There was a problem hiding this comment.
The IL for all of the added & changed tested methods (I haven't checked the pre-existing ones) look good to me at least now :)
|
Done with review pass (commit 6) #Closed |
|
Done with review pass (commit 7) #Closed |
I am not sure what was the point in replacing this override with Refers to: src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs:2050 in 3ffd5f5. [](commit_id = 3ffd5f5, deletion_comment = True) |
|
Done with review pass (commit 8) #Closed |
|
@dotnet/roslyn-compiler for another review, thanks |
| return node; | ||
| } | ||
|
|
||
| public override BoundNode VisitCall(BoundCall node) |
There was a problem hiding this comment.
What about BoundFunctionPointerInvocations? Do we need to return true when one is encountered? Or what if it's funcPtr(ref *otherPtr)?
There was a problem hiding this comment.
Retracking is only useful if a pointer is dereferenced and stored in a by-ref local. Function pointer invocation doesn't seem to apply to that.
But I think we can ignore pointer dereferences that are passed through a function pointer invocation, just like we do for normal invocation. I will do that (and add tests for function pointer invocations).
Fixes #82137.
Follow up on #79311 to handle more cases.
@hamarb123 @jkotas fyi
Some background
If there is a pointer-to-ref conversion, we need to preserve the by-ref local in IL so the CLR can "re-track" it, i.e., not garbage-collect it while it's in a managed ref-local. Previously we would sometimes optimize the local away so in IL the code would be just pointer manipulation which GC doesn't track.
For example, in Release config, this code
would be lowered to
but that's wrong if GC happens in between
p->Fand.ToString()(if CLR sees the former code it has to track that localband cannot collect it but not in the latter case). More real-world examples are in the linked issue. We previously fixed this for some cases but not for some more nested scenarios.