-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Emit and roundtrip underlying instance field #73407
Conversation
a215b6b
to
b78e1ea
Compare
@AlekseyTs @jjonescz for review. Thanks |
return default; | ||
} | ||
|
||
Debug.Assert(foundUnderlyingInstanceField.IsNil); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
underlyingType = type; | ||
// Validate instance field | ||
if (!underlyingInstanceField.IsNil | ||
&& !validateUnderlyingInstanceField(underlyingInstanceField, moduleSymbol, type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2046,6 +2086,26 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym | |||
Debug.Assert(underlyingType is not null); | |||
return true; | |||
} | |||
|
|||
bool validateUnderlyingInstanceField(FieldDefinitionHandle underlyingInstanceFieldHandle, PEModuleSymbol moduleSymbol, TypeSymbol underlyingType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's two reasons to care about the presence of the field:
- for
Unsafe.As
to be safe, we need the extension to be properly formed. We could do lighter validation (just the type) but I don't see the point to accept ill-formed extension types this.UnderlyingMember
in an instance extension type member will need to be rewritten to access this field, something likethis.<UnderlyingInstanceField>$.UnderlyingMember
(this will be in a later PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.UnderlyingMember
in an instance extension type member will need to be rewritten to access this field, something likethis.<UnderlyingInstanceField>$.UnderlyingMember
.
I don't think we will ever need to do something like this for an imported extension type. We will never be emitting code for its members
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct. That leaves reason 1
Does it make sense to continue after this point? #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs:2045 in 6aa7e12. [](commit_id = 6aa7e12, deletion_comment = False) |
try | ||
{ | ||
foreach (var fieldRid in module.GetFieldsOfTypeOrThrow(_handle)) | ||
{ | ||
try | ||
{ | ||
if (!underlyingInstanceField.IsNil && fieldRid == underlyingInstanceField) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (_lazyUnderlyingInstanceField is null) | ||
{ | ||
var field = new SynthesizedFieldSymbol(this, extendedType, WellKnownMemberNames.ExtensionFieldName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields in structs use LayoutKind.Sequential
by default. Extensions do the same:
internal sealed override TypeLayout Layout
{
get
{
var data = GetDecodedWellKnownAttributeData();
if (data != null && data.HasStructLayoutAttribute)
{
return data.Layout;
}
if (this.TypeKind is TypeKind.Struct or TypeKind.Extension)
{
// PROTOTYPE consider disallowing attribute for explicit layout on extension types
// CLI spec 22.37.16:
// "A ValueType shall have a non-zero size - either by defining at least one field, or by providing a non-zero ClassSize"
//
// Dev11 compiler sets the value to 1 for structs with no instance fields and no size specified.
// It does not change the size value if it was explicitly specified to be 0, nor does it report an error.
return new TypeLayout(LayoutKind.Sequential, this.HasInstanceFields() ? 0 : 1, alignment: 0);
}
return default(TypeLayout);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fields in structs use
LayoutKind.Sequential
by default.
Could you point me to a documentation confirming that the first instance field will start at offset 0 then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From ECMA CLI (II.10.1.2):
sequential: The CLI shall lay out the fields in sequential order, based on the order of the fields in the logical metadata table (§II.22.15).
[Rationale: [...] sequential layout is intended to instruct the CLI to match layout rules commonly followed by languages like C and C++ on an individual platform, where this is possible while still guaranteeing verifiable layout. [...]
From the C99 standard section 6.7.2.1 bullet point 13 (unfortunately, gated):
Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning.
|
||
if (!type.IsStatic) | ||
{ | ||
var fieldDef = reader.GetFieldDefinition(fieldDefHandle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That is a nice simplification indeed
@@ -54,7 +57,7 @@ private static void AssertSetStrictlyEqual(string[] expected, string[] actual) | |||
} | |||
|
|||
// Verify things that are common for all extension types | |||
private static void VerifyExtension<T>(TypeSymbol type, bool? isExplicit, SpecialType specialType = SpecialType.None) where T : TypeSymbol | |||
private static void VerifyExtension<T>(TypeSymbol type, bool? isExplicit, SpecialType specialType = SpecialType.None, string fieldType = null) where T : TypeSymbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var fieldTypeDisplay = decoder.DecodeFieldSignature(ref blob); | ||
|
||
// The instance value field has the expected type | ||
Assert.Equal(fieldType, fieldTypeDisplay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow. What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow. What do you mean?
I mean validateUnderlyingInstanceField
function checks more things than this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added missing checks
Are details of the field that we are supposed to emit specified anywhere? #Resolved |
Done with review pass (commit 1) |
@@ -18,6 +20,7 @@ | |||
using Roslyn.Test.Utilities; | |||
using Roslyn.Utilities; | |||
using Xunit; | |||
using static Roslyn.Test.Utilities.MetadataReaderUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this using necessary? #Closed
if (mrEx is not null | ||
|| signatureHeader.IsGeneric | ||
|| paramInfos.Length <= 1 | ||
|| this.Layout is not { Kind: LayoutKind.Sequential, Alignment: 0, Size: 0 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| this.Layout is not { Kind: LayoutKind.Sequential, Alignment: 0, Size: 0 })
It feels like this can be checked even before we call to tryDecodeExtensionType
. I think that one looking for this check will have a very hard time finding it because it is unlikely that one would expect to find the check where the method's signature is verified. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this location is find for this check. This method does not only concern itself with the method's signature.
I could move it earlier in the method if that helps.
The alternative would be putting the check in TryGetExtensionInfo
, but then an unexpected layout would result in the type not being considered an extension at all.
The boundary between "not an extension" and a "bad extension" is fungible, but the layout check feels more like the latter category.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think you're suggesting to do the check in this method, but before the local function is called. That's fine too
Assert.Equal(Accessibility.Private, peField.DeclaredAccessibility); | ||
Assert.False(peField.IsStatic); | ||
Assert.False(peField.IsReadOnly); | ||
Assert.Equal(RefKind.None, peField.RefKind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with review pass (commit 5) |
Opened corresponding spec PR dotnet/csharplang#8121 In reply to: 2108098258 |
@@ -1954,6 +1984,14 @@ private void DecodeExtensionType(out bool isExplicit, out TypeSymbol underlyingT | |||
// PROTOTYPE consider optimizing/caching | |||
|
|||
TypeSymbol? foundUnderlyingType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this declaration after the if
since it's not used in there. #Resolved
@@ -3198,7 +3228,45 @@ public class C<T> { } | |||
var comp = CreateCompilation(src, targetFramework: TargetFramework.Net70); | |||
comp.VerifyDiagnostics(); | |||
|
|||
CompileAndVerify(comp, symbolValidator: validate, sourceSymbolValidator: validate, verify: Verification.FailsPEVerify); | |||
var verifier = CompileAndVerify(comp, symbolValidator: validate, sourceSymbolValidator: validate, verify: Verification.FailsPEVerify); | |||
// Note: we don't emit a DynamicAttribute on synthesized field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in the speclet? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a note there
73 00 00 | ||
) | ||
// Fields | ||
.field private class C`1<object> '<UnderlyingInstance>$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'<UnderlyingInstance>$'
Did you want to use {{WellKnownMemberNames.ExtensionFieldName}}
here like in most other tests? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if runtime team agrees this representation is fine
{ | ||
var module = (PEModuleSymbol)type.ContainingModule; | ||
var reader = module.Module.GetMetadataReader(); | ||
var fieldDefHandle = reader.GetTypeDefinition(peType.Handle).GetFields() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 7)
…net#73407)" This reverts commit 3dc87ab.
Relates to test plan #66722
Corresponding spec details: dotnet/csharplang#8121