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.

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

Allow initonly field during ldflda and stfld import #57385

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Allow initonly field during ldflda and stfld import #57385

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
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ private bool TryGetUnmanagedCallingConventionFromModOpt(MethodSignature signatur
// Default to managed since in the modopt case we need to differentiate explicitly using a calling convention that matches the default
// and not specifying a calling convention at all and using the implicit default case in P/Invoke stub inlining.
callConv = CorInfoCallConvExtension.Managed;
if (!signature.HasEmbeddedSignatureData || signature.GetEmbeddedSignatureData() == null)
if (!signature.HasEmbeddedSignatureData)
return false;

bool found = false;
Expand Down
34 changes: 25 additions & 9 deletions src/coreclr/tools/ILVerification/ILImporter.Verify.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,9 +267,9 @@ private void FindEnclosingExceptionRegions()
}
}
// Check if offset is within the range [FilterOffset, HandlerOffset[
if (r.FilterOffset != -1 && r.FilterOffset <= offset && offset < r.HandlerOffset )
if (r.FilterOffset != -1 && r.FilterOffset <= offset && offset < r.HandlerOffset)
{
if(!basicBlock.FilterIndex.HasValue)
if (!basicBlock.FilterIndex.HasValue)
{
basicBlock.FilterIndex = j;
}
Expand Down Expand Up @@ -301,7 +301,7 @@ private void InitialPass()
ILOpcode opCode = (ILOpcode)ReadILByte();

previousWasPrefix = false;
again:
again:
switch (opCode)
{
// Check this pointer modification
Expand Down Expand Up @@ -1601,7 +1601,7 @@ void ImportCall(ILOpcode opcode, int token)
}

// To support direct calls on readonly byrefs, just pretend declaredThis is readonly too
if(declaredThis.Kind == StackValueKind.ByRef && (actualThis.Kind == StackValueKind.ByRef && actualThis.IsReadOnly))
if (declaredThis.Kind == StackValueKind.ByRef && (actualThis.Kind == StackValueKind.ByRef && actualThis.IsReadOnly))
{
declaredThis.SetIsReadOnly();
}
Expand Down Expand Up @@ -2107,8 +2107,9 @@ void ImportAddressOfField(int token, bool isStatic)
isPermanentHome = actualThis.Kind == StackValueKind.ObjRef || actualThis.IsPermanentHome;
instance = actualThis.Type;

if (field.IsInitOnly)
Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly);
// TODO: verification of readonly references https://github.com/dotnet/runtime/issues/57444
// if (field.IsInitOnly)
am11 marked this conversation as resolved.
Show resolved Hide resolved
// Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly);
}

Check(_method.OwningType.CanAccess(field, instance), VerifierError.FieldAccess);
Expand All @@ -2122,10 +2123,9 @@ void ImportStoreField(int token, bool isStatic)
ClearPendingPrefix(Prefix.Volatile);

var value = Pop();

var field = ResolveFieldToken(token);

TypeDesc instance;

if (isStatic)
{
Check(field.IsStatic, VerifierError.ExpectedStaticField);
Expand Down Expand Up @@ -2154,7 +2154,8 @@ void ImportStoreField(int token, bool isStatic)
instance = actualThis.Type;

if (field.IsInitOnly)
Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly);
Check(field.OwningType == _method.OwningType && actualThis.IsThisPtr &&
(_method.IsConstructor || HasIsExternalInit(_method.Signature)), VerifierError.InitOnly);
}

// Check any constraints on the fields' class --- accessing the field might cause a class constructor to run.
Expand Down Expand Up @@ -2647,6 +2648,21 @@ void CheckPendingPrefix(Prefix mask)
Check((mask & Prefix.Constrained) == 0, VerifierError.Constrained);
}

static bool HasIsExternalInit(MethodSignature signature)
{
if (signature.HasEmbeddedSignatureData)
{
foreach (var data in signature.GetEmbeddedSignatureData())
{
if (data.type is MetadataType mdType && mdType.Namespace == "System.Runtime.CompilerServices" && mdType.Name == "IsExternalInit" &&
data.index == MethodSignature.IndexOfCustomModifiersOnReturnType)
return true;
}
}

return false;
}

bool HasPendingPrefix(Prefix prefix)
{
return (_pendingPrefix & prefix) != 0;
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.TypeSystem.ReadyToRun.Tests/SignatureTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ public SignatureTests(ITestOutputHelper output)

private static string GetModOptMethodSignatureInfo(MethodSignature signature)
{
if (!signature.HasEmbeddedSignatureData || signature.GetEmbeddedSignatureData() == null)
return "";
if (!signature.HasEmbeddedSignatureData)
am11 marked this conversation as resolved.
Show resolved Hide resolved
return string.Empty;

StringBuilder sb = new StringBuilder();
foreach (EmbeddedSignatureData data in signature.GetEmbeddedSignatureData())
Expand Down
34 changes: 23 additions & 11 deletions src/tests/ilverify/ILTests/FieldTests.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@
.field public initonly int32 InstanceInitonlyField
.field public static initonly int32 StaticInitonlyField

.field private initonly int32 InstanceExternalInitField

.method public hidebysig specialname instance void modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) 'special.StoreExternalInitField.set_MyProperty_Valid'(int32 'value') cil managed { ret }
.method public hidebysig specialname instance void modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) set_MyProperty(int32 'value') cil managed
{
ldarg.0
ldarg.1
stfld int32 FieldTestsType::InstanceExternalInitField
ret
}

.method public instance void Stsfld.UnsatisfiedParentConstraints_Invalid_UnsatisfiedFieldParentInst() cil managed
{
ldc.i4.0
Expand All @@ -43,7 +54,8 @@
ret
}

.method public instance void Ldflda.InitonlyFieldOutsideCtor_Invalid_InitOnly() cil managed
// TODO: verification of readonly references https://github.com/dotnet/runtime/issues/57444
.method public instance void Ldflda.InitonlyFieldOutsideCtor_Valid() cil managed
am11 marked this conversation as resolved.
Show resolved Hide resolved
{
ldarg.0
ldflda int32 FieldTestsType::InstanceInitonlyField
Expand All @@ -52,7 +64,7 @@
}

.method public hidebysig instance void 'special.StoreInitonlyField..ctor_Valid'() cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor() cil managed
.method public hidebysig specialname rtspecialname instance void .ctor() cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -63,7 +75,7 @@
}

.method public hidebysig instance void 'special.LoadAddrInitonlyField..ctor_Valid'(int32) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(int32) cil managed
.method public hidebysig specialname rtspecialname instance void .ctor(int32) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -74,7 +86,7 @@
}

.method public hidebysig instance void 'special.LoadAddrInitonlyField..ctor_Valid'(int64) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(int64) cil managed
.method public hidebysig specialname rtspecialname instance void .ctor(int64) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -85,7 +97,7 @@
}

.method public hidebysig instance void 'special.StoreInitonlyFieldOtherType..ctor_Invalid_InitOnly'(class OtherType c) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(class OtherType c) cil managed
.method public hidebysig specialname rtspecialname instance void .ctor(class OtherType c) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -96,7 +108,7 @@
}

.method public hidebysig instance void 'special.StoreInitonlyFieldOtherInstance..ctor_Invalid_InitOnly'(class FieldTestsType c) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(class FieldTestsType c) cil managed
Copy link
Member

Choose a reason for hiding this comment

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

body is actually not doing what method name suggests.

As far as I can tell, StoreInitonlyFieldOtherInstance described well what the body was doing. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ops, you are right. I mixed up the "other instance" vs. "other type" instance for which we have StoreInitonlyFieldOtherType.

.method public hidebysig specialname rtspecialname instance void .ctor(class FieldTestsType c) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -107,7 +119,7 @@
}

.method public hidebysig instance void 'special.StsfldInitonlyInCtor..ctor_Invalid_InitOnly'(bool) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(bool) cil managed
.method public hidebysig specialname rtspecialname instance void .ctor(bool) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -117,7 +129,7 @@
}

.method public hidebysig instance void 'special.LdsfldInitonlyInCtor..ctor_Invalid_InitOnly'(char) cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .ctor(char) cil managed
.method public hidebysig specialname rtspecialname instance void .ctor(char) cil managed
{
ldarg.0
call instance void [System.Runtime]System.Object::.ctor()
Expand All @@ -127,7 +139,7 @@
}

.method public hidebysig instance void 'special.LdsfldStslfdInitonlyCctor..cctor_Valid'() cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .cctor() cil managed
.method public hidebysig specialname rtspecialname instance void .cctor() cil managed
{
ldsflda int32 FieldTestsType::StaticInitonlyField
pop
Expand All @@ -145,8 +157,8 @@
.field public initonly int32 InstanceInitonlyField
.field public static initonly int32 StaticInitonlyField

.method public hidebysig instance void 'special.LdfldStlfdInitonlyCctor..cctor_Invalid_InitOnly.InitOnly'() cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .cctor() cil managed
.method public hidebysig instance void 'special.LdfldStlfdInitonlyCctor..cctor_Invalid_InitOnly'() cil managed { ret }
.method public hidebysig specialname rtspecialname instance void .cctor() cil managed
{
ldsfld class OtherType OtherType::Instance
ldflda int32 OtherType::InstanceInitonlyField
Expand Down
2 changes: 1 addition & 1 deletion src/tests/ilverify/ILTests/FunctionPointerTests.il
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
ret
}

.method public hidebysig instance void Call.SendFunctionPointerWithInaccessibleReturnType_Invalid_MethodAccess_MethodAccess() cil managed
.method public hidebysig instance void Call.SendFunctionPointerWithInaccessibleReturnType_Invalid_MethodAccess.MethodAccess() cil managed
{
newobj instance void C::.ctor()
ldftn class D/D_Private D::StaticMethodReturningPrivateTypeInstance(int32)
Expand Down
51 changes: 34 additions & 17 deletions src/tests/ilverify/TestDataLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
namespace ILVerification.Tests
{
/// <summary>
/// Parses the methods in the test assemblies.
/// Parses the methods in the test assemblies.
/// It loads all assemblies from the test folder defined in <code>TestDataLoader.TestAssemblyPath</code>
/// This class feeds the xunit Theories
/// </summary>
Expand Down Expand Up @@ -106,8 +106,8 @@ private static TheoryData<TestCase> GetTestTypeFromDll(Func<string[], TypeDefini
/// <summary>
/// Returns all methods that contain valid IL code based on the following naming convention:
/// [FriendlyName]_Valid
/// The method must contain 1 '_'. The part before the '_' is a friendly name describing what the method does.
/// The word after the '_' has to be 'Valid' (Case sensitive)
/// The method must contain 1 '_'. The part before the '_' is a friendly name describing what the method does.
/// The word after the '_' has to be 'Valid' (Case sensitive)
/// E.g.: 'SimpleAdd_Valid'
/// </summary>
public static TheoryData<TestCase> GetMethodsWithValidIL()
Expand All @@ -129,9 +129,9 @@ public static TheoryData<TestCase> GetMethodsWithValidIL()
/// The method name must contain 2 '_' characters.
/// 1. part: a friendly name
/// 2. part: must be the word 'Invalid' (Case sensitive)
/// 3. part: the expected VerifierErrors as string separated by '.'.
/// 3. part: the expected VerifierErrors as string separated by '.'.
/// E.g.: SimpleAdd_Invalid_ExpectedNumericType
/// </summary>
/// </summary>
public static TheoryData<TestCase> GetMethodsWithInvalidIL()
{
var methodSelector = new Func<string[], MethodDefinitionHandle, TestCase>((mparams, methodHandle) =>
Expand Down Expand Up @@ -176,20 +176,37 @@ private static TheoryData<TestCase> GetTestMethodsFromDll(Func<string[], MethodD
var method = (EcmaMethod)testModule.GetMethod(methodHandle);
var methodName = method.Name;

if (!String.IsNullOrEmpty(methodName) && methodName.Contains("_"))
if (!methodName.Contains('_', StringComparison.Ordinal))
continue;

var index = methodName.LastIndexOf("_Valid", StringComparison.Ordinal);
if (index < 0)
index = methodName.LastIndexOf("_Invalid", StringComparison.Ordinal);
if (index < 0)
continue;

am11 marked this conversation as resolved.
Show resolved Hide resolved
var substring = methodName.Substring(index + 1);
var split = substring.Split('_');
string[] mparams = new string[split.Length + 1];
split.CopyTo(mparams, 1);
mparams[0] = methodName.Substring(0, index);
// examples of methodName to mparams transformation:
// * `get_Property` -> [ 'get_Property' ]
// * `CheckSomething_Valid` -> [ 'CheckSomething', 'Valid' ]
// * 'WrongMethod_Invalid_BranchOutOfTry' -> [ 'WrongMethod', 'Invalid', 'BranchOutOfTry' ]
// * 'MoreWrongMethod_Invalid_TypeAccess.InitOnly' -> [ 'MoreWrongMethod', 'Invalid', 'TypeAccess', 'InitOnly' ]
Copy link
Member

Choose a reason for hiding this comment

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

TypeAccess.InitOnly

Should this be TypeAccess_InitOnly? I do not see where we are recognizing . as the separator.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have 25 cases using period as a error code separator when there is more than one error:

$ git grep '_Invalid_.*\..*\(.*\)' ':/src/tests/ilverify/ILTests'

It is documented here https://github.com/dotnet/runtime/tree/main/src/coreclr/tools/ILVerify#methods-with-special-names. I agree it is quite complicated; imo, we should use xml or json list for this rather than complex naming convention parsing.

Copy link
Member

Choose a reason for hiding this comment

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

we should use xml or json list

Then you have a problem with the IL and json being out-of-sync all the time. I actually like the convention based scheme we have here.

Copy link
Member Author

@am11 am11 Aug 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Then you have a problem with the IL and json being out-of-sync all the time.

Current approach is equally fragile. The test discovery does not give any indication whether or not it has skipped the test during parsing (which is spread across three separate methods of TestDataLoader.cs). We have to remember total count before and after the new additions.

// * 'special.set_MyProperty.set_MyProperty_Invalid_InitOnly' -> [ 'special.set_MyProperty.set_MyProperty', 'Invalid', 'InitOnly' ]

var specialMethodHandle = HandleSpecialTests(mparams, method);
var newItem = methodSelector(mparams, specialMethodHandle);

if (newItem != null)
{
var mparams = methodName.Split('_');
var specialMethodHandle = HandleSpecialTests(mparams, method);
var newItem = methodSelector(mparams, specialMethodHandle);

if (newItem != null)
{
newItem.TestName = mparams[0];
newItem.MethodName = methodName;
newItem.ModuleName = testDllName;
newItem.TestName = mparams[0];
newItem.MethodName = methodName;
newItem.ModuleName = testDllName;

retVal.Add(newItem);
}
retVal.Add(newItem);
}
}
}
Expand Down