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

Add support for UnsafeAccessorKind.FieldOffset (#45152) #93946

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 41 additions & 14 deletions src/coreclr/vm/prestub.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,8 @@ namespace
Method, // call instance method (`callvirt` in IL)
StaticMethod, // call static method (`call` in IL)
Field, // address of instance field (`ldflda` in IL)
StaticField // address of static field (`ldsflda` in IL)
StaticField, // address of static field (`ldsflda` in IL)
FieldOffset, // offset of field relative to the start of the object
};

bool TryParseUnsafeAccessorAttribute(
Expand Down Expand Up @@ -1316,7 +1317,8 @@ namespace
STANDARD_VM_CONTRACT;
_ASSERTE(fieldName != NULL);
_ASSERTE(cxt.Kind == UnsafeAccessorKind::Field
|| cxt.Kind == UnsafeAccessorKind::StaticField);
|| cxt.Kind == UnsafeAccessorKind::StaticField
|| cxt.Kind == UnsafeAccessorKind::FieldOffset);

TypeHandle targetType = cxt.TargetType;
_ASSERTE(!targetType.IsTypeDesc());
Expand All @@ -1325,15 +1327,30 @@ namespace
targetType.AsMethodTable(),
(cxt.IsTargetStatic ? ApproxFieldDescIterator::STATIC_FIELDS : ApproxFieldDescIterator::INSTANCE_FIELDS));
PTR_FieldDesc pField;
while ((pField = fdIterator.Next()) != NULL)

if (cxt.Kind == UnsafeAccessorKind::FieldOffset)
{
// Validate the name and target type match.
if (strcmp(fieldName, pField->GetName()) == 0
&& fieldType == pField->LookupFieldTypeHandle())
{
cxt.TargetField = pField;
return true;
}
while ((pField = fdIterator.Next()) != NULL)
{
// Validate the name and target type match.
if (strcmp(fieldName, pField->GetName()) == 0)
{
cxt.TargetField = pField;
return true;
}
}
}
else
{
while ((pField = fdIterator.Next()) != NULL)
{
// Validate the name and target type match.
if (strcmp(fieldName, pField->GetName()) == 0 && fieldType == pField->LookupFieldTypeHandle())
{
cxt.TargetField = pField;
return true;
}
}
}
return false;
}
Expand Down Expand Up @@ -1368,8 +1385,13 @@ namespace
// during dispatch.
UINT beginIndex = cxt.IsTargetStatic ? 1 : 0;
UINT stubArgCount = cxt.DeclarationSig.NumFixedArgs();
for (UINT i = beginIndex; i < stubArgCount; ++i)
pCode->EmitLDARG(i);

// Don't load the first argument for a field offset, we are not using it.
if (cxt.Kind != UnsafeAccessorKind::FieldOffset)
{
for (UINT i = beginIndex; i < stubArgCount; ++i)
pCode->EmitLDARG(i);
}

// Provide access to the target member
UINT targetArgCount = stubArgCount - beginIndex;
Expand All @@ -1396,6 +1418,10 @@ namespace
_ASSERTE(cxt.TargetField != NULL);
pCode->EmitLDSFLDA(pCode->GetToken(cxt.TargetField));
break;
case UnsafeAccessorKind::FieldOffset:
_ASSERTE(cxt.TargetField != NULL);
pCode->EmitLDC(cxt.TargetField->GetOffset());
break;
default:
_ASSERTE(!"Unknown UnsafeAccessorKind");
}
Expand Down Expand Up @@ -1519,16 +1545,17 @@ bool MethodDesc::TryGenerateUnsafeAccessor(DynamicResolver** resolver, COR_ILMET

case UnsafeAccessorKind::Field:
case UnsafeAccessorKind::StaticField:
case UnsafeAccessorKind::FieldOffset:
// Field access requires a single argument for target type and a return type.
if (argCount != 1 || firstArgType.IsNull() || context.DeclarationSig.IsReturnTypeVoid())
{
ThrowHR(COR_E_BADIMAGEFORMAT, BFA_INVALID_UNSAFEACCESSOR);
}

// The return type must be byref.
// The return type must be byref for field access.
// If the non-static field access is for a
// value type, the instance must be byref.
if (!retType.IsByRef()
if ((!retType.IsByRef() && kind != UnsafeAccessorKind::FieldOffset)
|| (kind == UnsafeAccessorKind::Field
&& firstArgType.IsValueType()
&& !firstArgType.IsByRef()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@ public enum UnsafeAccessorKind
/// <summary>
/// Provide access to a static field.
/// </summary>
StaticField
StaticField,

/// <summary>
/// Provide access to the offset of an instance field.
/// </summary>
FieldOffset
};

/// <summary>
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13304,7 +13304,8 @@ public enum UnsafeAccessorKind
Method,
StaticMethod,
Field,
StaticField
StaticField,
FieldOffset
};
[System.AttributeUsageAttribute(System.AttributeTargets.Struct)]
public sealed partial class UnsafeValueTypeAttribute : System.Attribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class UserDataClass
{
public const string StaticFieldName = nameof(_F);
public const string FieldName = nameof(_f);
public const string FieldSecondName = nameof(_i);
public const string StaticMethodName = nameof(_M);
public const string MethodName = nameof(_m);
public const string StaticMethodVoidName = nameof(_MVV);
Expand All @@ -30,6 +31,7 @@ class UserDataClass

private static string _F = PrivateStatic;
private string _f;
private int _i;

public string Value => _f;

Expand Down Expand Up @@ -215,6 +217,18 @@ public static void Verify_AccessFieldClass()
extern static ref string GetPrivateField(UserDataClass d);
}

[Fact]
public static void Verify_AccessFieldOffsetClass()
{
Console.WriteLine($"Running {nameof(Verify_AccessFieldOffsetClass)}");

// Offset = method table pointer + first string pointer
Assert.Equal(IntPtr.Size + IntPtr.Size, GetPrivateFieldOffset());
Copy link
Contributor

@hamarb123 hamarb123 Oct 26, 2023

Choose a reason for hiding this comment

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

This should just be IntPtr.Size I think, since you cannot legitimately obtain a ref before the first field iirc.
Presumably the GetRawData API will give the offset of the 0th field.
Also, should we have tests for structs and for ref fields?

(edit: I'm editing this because my edge crashed after posting my comments and undid all my changes & deleted a comment?!)


[UnsafeAccessor(UnsafeAccessorKind.FieldOffset, Name=UserDataClass.FieldSecondName)]
extern static nint GetPrivateFieldOffset(UserDataClass? d = null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what others' opinions on this are, but I like to be explicit with these sorts of APIs - I'd like it if we could also specify a second parameter which is the type of the field - it would first check for fields with same type (including modifiers), and then same type (ignoring modifiers) (same as other checks for the UnsafeAccessors iirc).

}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/92633")]
public static void Verify_AccessStaticFieldGenericClass()
Expand Down
Loading