Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API proposal: Obsolete RuntimeHelpers.OffsetToStringData #31406

Closed
GrabYourPitchforks opened this issue Nov 6, 2019 · 10 comments · Fixed by #35722
Closed

API proposal: Obsolete RuntimeHelpers.OffsetToStringData #31406

GrabYourPitchforks opened this issue Nov 6, 2019 · 10 comments · Fixed by #35722
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

API proposal

namespace System.Runtime.CompilerServices
{
    public sealed class RuntimeHelpers
    {
        [Obsolete("Use string.GetPinnableReference() instead.")]
        public static int OffsetToStringData { get; }
    }
}

Justification

The RuntimeHelpers.OffsetToStringData property is the last remaining public API which exposes the concept of the string object containing inline UTF-16 data just after the object header. It's used by compilers to pin string instances, generally (but not always) using the following pseudocode:

fixed string fixedLocal = theStringInstance;
char* pchStr = (char*)((byte*)theStringInstance + RuntimeHelpers.OffsetToStringData);
// use 'pchStr' here

Because of this pattern throughout existing libraries, adding support for things like string compaction (where the backing data might be ASCII instead of UTF-16) becomes difficult and error-prone.

In .NET Core 3.0, we added an API string.GetPinnableReference() which takes advantage of the new pattern-based fixed statement support added in C# 7.3. This new API would allow us to intercept the call immediately before the pin operation, allowing the runtime to fix up the data as necessary. When targeting netcoreapp3.0, the compiler will prefer the new GetPinnableReference API over the old RuntimeHelpers API.

As more and more DLLs are built which target netcoreapp3.0+, we should gradually see the ecosystem move over to the new API, allowing us to experiment with changing the internal layout of the string type once a critical mass of DLLs has migrated away from the old APIs.

Since the main consumer of the old API is the C# compiler itself, I don't anticipate most code bases seeing new warnings after upgrade. The compiler will automatically prefer the new API anyway. The reason I propose obsoleting the old API is so tooling authors and devs writing ref emit code or otherwise manipulating IL directly will see the warning and know that they should instead be calling a new more future-proof API.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@weltkante
Copy link
Contributor

The C++/CLI headers are also using this (vcclr.h uses OffsetToStringData in PtrToStringChars which is used throughout various other headers to implement various string conversion helpers).

@GrabYourPitchforks GrabYourPitchforks added the blocking Marks issues that we want to fast track in order to unblock other important work label May 6, 2020
@terrajobst
Copy link
Member

When targeting netcoreapp3.0, the compiler will prefer the new GetPinnableReference API over the old RuntimeHelpers API.

Have we validated that VB and F# don't use this API either? What about C++/CLI?

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented May 20, 2020

F#: Opened dotnet/fsharp#9251 to track the compiler work.

VB: I cannot figure out how to pin a string in VB. Can somebody confirm that the language even has compiler support?

C++/CLI: I'll open a work item if somebody tells me where to do so. Can somebody confirm that C++/CLI is supported on .NET 5.0+?

FWIW I wouldn't hold up the obsoletion work while waiting for these. At this point we're talking about the IL that the compilers spew, which I can't fathom would be broken by non-runtime-observable obsoletion of API surface.

@weltkante
Copy link
Contributor

Can somebody confirm that C++/CLI is supported on .NET 5.0+?

WPF is using it actively

@terrajobst
Copy link
Member

terrajobst commented May 21, 2020

C++/CLI: I'll open a work item if somebody tells me where to do so. Can somebody confirm that C++/CLI is supported on .NET 5.0+?

It's supported for Windows and (as @weltkante said) is used by WPF.

@terrajobst
Copy link
Member

VB: I cannot figure out how to pin a string in VB. Can somebody confirm that the language even has compiler support?

@jaredpar

@cartermp
Copy link

Work in F# is here: dotnet/fsharp#9252

@GrabYourPitchforks
Copy link
Member Author

The earlier VB comment should've read "Can somebody confirm that VB even has support for pinning strings?" My brain is taking a long weekend.

@danmoseley
Copy link
Member

VB can't use unsafe code right?

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels May 26, 2020
@terrajobst
Copy link
Member

  • Looks good.
namespace System.Runtime.CompilerServices
{
    public sealed class RuntimeHelpers
    {
        [Obsolete("Use string.GetPinnableReference() instead.")]
        public static int OffsetToStringData { get; }
    }
}

@ghost ghost closed this as completed in #35722 Jun 9, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants