[WIP] Look for string.GetPinnableReference when pinning a string first #9252
Conversation
|
Thanks kindly for getting to this so quickly! :) |
| // | ||
| // let ptr: nativeptr<char> = | ||
| // let pinned s = str | ||
| // (nativeptr)s + GetPinnableReference() |
There was a problem hiding this comment.
Just to clarify, the compiler shouldn't perform any arithmetic here. The result of GetPinnableReference is a readonly char&, and that char& should be pinned. The string instance itself should not be pinned.
If you take a look at the "expected IL" example in #9251, the result of GetPinnableReference is immediately assigned to the pinned char& local. There's no add instruction in the expected IL.
There was a problem hiding this comment.
Hmmm. @dsyme wrote this originally, I assume he had a good reason for doing this, but yes it shouldn't need to do that.
There was a problem hiding this comment.
Clarification to my comments: the GetOffsetToStringData pattern works by having the caller pin the string instance, then compute addrof(string) + GetOffsetToStringData, then expose the resulting sum as a char*. So the addition is expected there.
The GetPinnableReference pattern works by providing the char& directly, which the caller then pins.
There was a problem hiding this comment.
Gotcha, I'll take another look at this
| ptr | ||
| """ | ||
| let il = """ | ||
| char& modreq([System.Runtime]System.Runtime.InteropServices.InAttribute) [System.Runtime]System.String::GetPinnableReference() |
There was a problem hiding this comment.
We should check to make sure the local char is pinned as well from locals init.
There was a problem hiding this comment.
Yeah, thinking about it more this is a bad test. I'll revisit this one
cartermp
changed the title
|
The fix here isn't right and will require a bit more work in the typechecker to do the right thing. I'm going to close this one out, at least until some of the test refactoring is completed, so that I don't have to baby the PR along without a proper fix. |
This tries to address #9251
I couldn't actually run these tests on my machine with VS for some weird reason, so I'll let CI tell me what I did wrong instead