Skip to content

Obsolete RuntimeHelpers.OffsetToStringData #35722

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.

Sign up for GitHub

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

Merged
1 commit merged into from
Jun 9, 2020
Merged

Obsolete RuntimeHelpers.OffsetToStringData #35722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 30 additions & 17 deletions src/coreclr/src/tools/Common/TypeSystem/Interop/IL/Marshaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1447,31 +1447,44 @@ protected override void TransformManagedToNative(ILCodeStream codeStream)
if (ShouldBePinned)
{
//
// Pin the string and push a pointer to the first character on the stack.
// Pin the char& and push a pointer to the first character on the stack.
//
TypeDesc stringType = Context.GetWellKnownType(WellKnownType.String);
TypeDesc charRefType = Context.GetWellKnownType(WellKnownType.Char).MakeByRefType();

ILLocalVariable vPinnedString = emitter.NewLocal(stringType, true);
ILCodeLabel lNullString = emitter.NewCodeLabel();
ILLocalVariable vPinnedCharRef = emitter.NewLocal(charRefType, true);

LoadManagedValue(codeStream);
codeStream.EmitStLoc(vPinnedString);
codeStream.EmitLdLoc(vPinnedString);
ILCodeLabel lNonNullString = emitter.NewCodeLabel();
ILCodeLabel lCommonExit = emitter.NewCodeLabel();

codeStream.Emit(ILOpcode.conv_i);
codeStream.Emit(ILOpcode.dup);
LoadManagedValue(codeStream);
codeStream.Emit(ILOpcode.brtrue, lNonNullString);

// Marshalling a null string?
codeStream.Emit(ILOpcode.brfalse, lNullString);
//
// Null input case
// Don't pin anything - load a zero-value nuint (void*) onto the stack
//
codeStream.Emit(ILOpcode.ldc_i4_0);
codeStream.Emit(ILOpcode.conv_u);
codeStream.Emit(ILOpcode.br, lCommonExit);

//
// Non-null input case
// Extract the char& from the string, pin it, then convert it to a nuint (void*)
//
codeStream.EmitLabel(lNonNullString);
LoadManagedValue(codeStream);
codeStream.Emit(ILOpcode.call, emitter.NewToken(
Context.SystemModule.
GetKnownType("System.Runtime.CompilerServices", "RuntimeHelpers").
GetKnownMethod("get_OffsetToStringData", null)));

codeStream.Emit(ILOpcode.add);
Context.GetWellKnownType(WellKnownType.String).
GetKnownMethod("GetPinnableReference", null)));
codeStream.EmitStLoc(vPinnedCharRef);
codeStream.EmitLdLoc(vPinnedCharRef);
codeStream.Emit(ILOpcode.conv_u);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be a downside to moving this conv_u to below lCommonExit? Then the duplicate one in the other branch could be removed.

Copy link
Member

@stephentoub stephentoub May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess maybe it's better for the same type to be on the stack prior to the branching?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the stack should be the same at all possible entry points to the branch. In one case the stack will have an i4, and in the other case the stack will have a char&. I was concerned that this would break things.

The codegen pattern here is identical to the codegen pattern Roslyn emits for pattern fixed statements. See Sharplab. Honestly I think some minor optimizations are possible, but I was nervous about straying from what the compiler itself already implements.


codeStream.EmitLabel(lNullString);
//
// Common exit
// Top of stack contains a nuint (void*) pointing to start of char data, or nullptr
//
codeStream.EmitLabel(lCommonExit);
StoreNativeValue(codeStream);
}
else
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9093,6 +9093,7 @@ public static partial class RuntimeFeature
}
public static partial class RuntimeHelpers
{
[System.ObsoleteAttribute("OffsetToStringData is obsolete. Use string.GetPinnableReference() instead.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be following @terrajobst's new plan for obsoletion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good question. :)

public static int OffsetToStringData { get { throw null; } }
public static IntPtr AllocateTypeAssociatedMemory(Type type, int size) { throw null; }
public static void EnsureSufficientExecutionStack() { }
Expand Down
16 changes: 0 additions & 16 deletions src/libraries/System.Runtime/tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,6 @@ public static unsafe void GetObjectValue()
Assert.Equal(i, (int)iOV);
}

[Fact]
public static unsafe void OffsetToStringData()
{
// RuntimeHelpers.OffsetToStringData
char[] expectedValues = new char[] { 'a', 'b', 'c', 'd', 'e', 'f' };
string s = "abcdef";

fixed (char* values = s) // Compiler will use OffsetToStringData with fixed statements
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed keyword doesn't call OffsetToStringData any more, so this test wasn't actually testing the right thing. I'm deleting this test since we already have coverage over in StringTests.cs:

runtime/src/libraries/System.Runtime/tests/System/StringTests.cs

Lines 871 to 895 in 61733e2

// Finally, ensure that GetPinnableReference matches the legacy 'fixed' keyword.
DynamicMethod dynamicMethod = new DynamicMethod("tester", typeof(bool), new[] { typeof(string) });
ILGenerator ilGen = dynamicMethod.GetILGenerator();
LocalBuilder pinnedLocal = ilGen.DeclareLocal(typeof(object), pinned: true);
ilGen.Emit(OpCodes.Ldarg_0); // load 'input' and pin it
ilGen.Emit(OpCodes.Stloc, pinnedLocal);
ilGen.Emit(OpCodes.Ldloc, pinnedLocal); // get the address of field 0 from pinned 'input'
ilGen.Emit(OpCodes.Conv_I);
ilGen.Emit(OpCodes.Call, typeof(RuntimeHelpers).GetProperty("OffsetToStringData").GetMethod); // get pointer to start of string data
ilGen.Emit(OpCodes.Add);
ilGen.Emit(OpCodes.Ldarg_0); // get value of input.GetPinnableReference()
ilGen.Emit(OpCodes.Callvirt, typeof(string).GetMethod("GetPinnableReference"));
// At this point, the top of the evaluation stack is traditional (fixed char* = input) and input.GetPinnableReference().
// Compare for equality and return.
ilGen.Emit(OpCodes.Ceq);
ilGen.Emit(OpCodes.Ret);
Assert.True((bool)dynamicMethod.Invoke(null, new[] { input }));

{
for (int i = 0; i < expectedValues.Length; i++)
{
Assert.Equal(expectedValues[i], values[i]);
}
}
}

[Fact]
public static void InitializeArray()
{
Expand Down