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

Jump to bottom

Emit and roundtrip underlying instance field #73407

Merged
merged 8 commits into from
May 15, 2024
Merged

Emit and roundtrip underlying instance field #73407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6aa7e12
Emit and roundtrip underlying instance field
jcouv May 7, 2024
9b00941
Address feedback
jcouv May 13, 2024
b892e3c
Add PROTOTYPE comment
jcouv May 13, 2024
bcc08a4
Validate layout too
jcouv May 13, 2024
3b18289
Address feedback
jcouv May 13, 2024
5fff14e
Address feedback
jcouv May 13, 2024
be50c30
Address feedback
jcouv May 14, 2024
cec3e26
Check there are no other instance fields
jcouv May 14, 2024
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
102 changes: 91 additions & 11 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1871,7 +1871,7 @@ public override TypeKind TypeKind
}

// PROTOTYPE consider caching/optimizing this computation
if (!TryGetExtensionMarkerMethod().IsNil)
if (!TryGetExtensionInfo().MarkerMethod.IsNil)
{
// Extension
result = TypeKind.Extension;
Expand All @@ -1895,10 +1895,12 @@ public override TypeKind TypeKind

/// <summary>
/// Superficially checks whether this is a valid extension type
/// and returns the extension marker method (to be validated later)
/// and returns the extension marker method and underlying instance field if applicable
/// if it is.
/// Both will be validated later.
/// </summary>
private MethodDefinitionHandle TryGetExtensionMarkerMethod()
private (MethodDefinitionHandle MarkerMethod, FieldDefinitionHandle UnderlyingInstanceField)
TryGetExtensionInfo()
{
var moduleSymbol = this.ContainingPEModule;
var module = moduleSymbol.Module;
Expand All @@ -1913,15 +1915,43 @@ private MethodDefinitionHandle TryGetExtensionMarkerMethod()

try
{
// They must not contain any instance state
// The only expected instance state is the underlying instance field
FieldDefinitionHandle foundUnderlyingInstanceField = default;
foreach (var field in module.GetFieldsOfTypeOrThrow(_handle))
{
if ((module.GetFieldDefFlagsOrThrow(field) & FieldAttributes.Static) == 0)
if (module.GetFieldDefNameOrThrow(field) == WellKnownMemberNames.ExtensionFieldName)
{
if ((module.GetFieldDefFlagsOrThrow(field) & FieldAttributes.Static) != 0)
{
// It must be an instance field
return default;
}

if (this.IsStatic)
{
// It's only allowed in non-static extension types
return default;
}

if (!foundUnderlyingInstanceField.IsNil)
{
return default;
}

foundUnderlyingInstanceField = field;
}
else if ((module.GetFieldDefFlagsOrThrow(field) & FieldAttributes.Static) == 0)
{
return default;
}
}

if (!this.IsStatic && foundUnderlyingInstanceField.IsNil)
{
// Non-static extensions must have an underlying instance field (to be validated later)
return default;
}

// They must have a single marker method (to be validated later)
MethodDefinitionHandle foundMarkerMethod = default;
foreach (var methodHandle in module.GetMethodsOfTypeOrThrow(_handle))
Expand All @@ -1940,7 +1970,7 @@ private MethodDefinitionHandle TryGetExtensionMarkerMethod()
}
}

return foundMarkerMethod;
return (foundMarkerMethod, foundUnderlyingInstanceField);
}
catch (BadImageFormatException)
{
Expand All @@ -1953,6 +1983,14 @@ private void DecodeExtensionType(out bool isExplicit, out TypeSymbol underlyingT
{
// PROTOTYPE consider optimizing/caching

if (!this.IsStatic
&& this.Layout is not { Kind: LayoutKind.Sequential, Alignment: 0, Size: 0 })
{
isExplicit = false;
underlyingType = ErrorTypeSymbol.UnknownResultType;
return;
}

TypeSymbol? foundUnderlyingType;
if (!tryDecodeExtensionType(out isExplicit, out foundUnderlyingType))
{
Expand All @@ -1968,10 +2006,11 @@ private void DecodeExtensionType(out bool isExplicit, out TypeSymbol underlyingT

bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSymbol? underlyingType)
{
var markerMethod = TryGetExtensionMarkerMethod();
var (markerMethod, underlyingInstanceField) = TryGetExtensionInfo();
Debug.Assert(!markerMethod.IsNil);
var moduleSymbol = this.ContainingPEModule;

// Decode and validate marker method
isExplicit = false;
underlyingType = null;

Expand All @@ -1987,7 +2026,7 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
}

// PROTOTYPE do we want to tighten the flags check further? (require that type be sealed?)
if ((localFlags & MethodAttributes.Private) == 0 ||
if ((localFlags & MethodAttributes.MemberAccessMask) != MethodAttributes.Private ||
(localFlags & MethodAttributes.Static) == 0)
{
return false;
Expand All @@ -1998,7 +2037,9 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
ParamInfo<TypeSymbol>[] paramInfos = new MetadataDecoder(moduleSymbol, context: this)
.GetSignatureForMethod(markerMethod, out signatureHeader, out mrEx);

if (mrEx is not null || signatureHeader.IsGeneric || paramInfos.Length <= 1)
if (mrEx is not null
|| signatureHeader.IsGeneric
|| paramInfos.Length <= 1)
{
return false;
}
Expand All @@ -2011,10 +2052,12 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
// PROTOTYPE need to decode extension type references (may be some cycle issues)
type = ApplyTransforms(type, paramInfo.Handle, moduleSymbol);

// PROTOTYPE consider checking top-level nullability annotation
if (paramInfo.IsByRef || !paramInfo.CustomModifiers.IsDefault)
{
var info = new CSDiagnosticInfo(ErrorCode.ERR_MalformedExtensionInMetadata, this); // PROTOTYPE need to report use-site diagnostic
type = new ExtendedErrorTypeSymbol(type, LookupResultKind.NotReferencable, info, unreported: true);
underlyingType = new ExtendedErrorTypeSymbol(type, LookupResultKind.NotReferencable, info, unreported: true);
return false;
}

if (i == 0)
Expand All @@ -2031,6 +2074,7 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
{
var info = new CSDiagnosticInfo(ErrorCode.ERR_MalformedExtensionInMetadata, this); // PROTOTYPE need to report use-site diagnostic
underlyingType = new ExtendedErrorTypeSymbol(type, LookupResultKind.NotReferencable, info, unreported: true);
return false;
}
else
{
Expand All @@ -2044,6 +2088,36 @@ bool tryDecodeExtensionType(out bool isExplicit, [NotNullWhen(true)] out TypeSym
}

Debug.Assert(underlyingType is not null);

if (!underlyingInstanceField.IsNil
&& !validateUnderlyingInstanceField(underlyingInstanceField, moduleSymbol, underlyingType))
{
var info = new CSDiagnosticInfo(ErrorCode.ERR_MalformedExtensionInMetadata, this); // PROTOTYPE need to report use-site diagnostic
underlyingType = new ExtendedErrorTypeSymbol(underlyingType, LookupResultKind.NotReferencable, info, unreported: true);
return false;
}

return true;
}

bool validateUnderlyingInstanceField(FieldDefinitionHandle underlyingInstanceFieldHandle, PEModuleSymbol moduleSymbol, TypeSymbol underlyingType)
Copy link
Contributor

@AlekseyTs AlekseyTs May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

bool validateUnderlyingInstanceField(FieldDefinitionHandle underlyingInstanceFieldHandle, PEModuleSymbol moduleSymbol, TypeSymbol underlyingType)

It is not obvious to me why do we care if the field is present and why do we need to perform any validation for it. #Closed

Copy link
Member Author

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:

  1. 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
  2. this.UnderlyingMember in an instance extension type member will need to be rewritten to access this field, something like this.<UnderlyingInstanceField>$.UnderlyingMember (this will be in a later PR)

Copy link
Contributor

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 like this.<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

Copy link
Member Author

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

{
var fieldSymbol = new PEFieldSymbol(moduleSymbol, this, underlyingInstanceFieldHandle);

if (fieldSymbol.TypeLayoutOffset != null
|| fieldSymbol.DeclaredAccessibility != Accessibility.Private
|| fieldSymbol.RefKind != RefKind.None
|| fieldSymbol.IsReadOnly)
{
return false;
}

if (!fieldSymbol.TypeWithAnnotations.Equals(TypeWithAnnotations.Create(underlyingType), TypeCompareKind.CLRSignatureCompareOptions))
{
return false;
}

// PROTOTYPE do we want to tighten the checks further? (required)
return true;
}
}
Expand Down Expand Up @@ -2192,6 +2266,12 @@ private MultiDictionary<string, PEFieldSymbol> CreateFields(ArrayBuilder<PEField
{
try
{
if (this.IsExtension
&& (module.GetFieldDefFlagsOrThrow(fieldRid) & FieldAttributes.Static) == 0)
{
continue;
}

if (!(isOrdinaryEmbeddableStruct ||
(isOrdinaryStruct && (module.GetFieldDefFlagsOrThrow(fieldRid) & FieldAttributes.Static) == 0) ||
module.ShouldImportField(fieldRid, moduleSymbol.ImportOptions)))
Expand Down Expand Up @@ -2231,7 +2311,7 @@ private PooledDictionary<MethodDefinitionHandle, PEMethodSymbol> CreateMethods(A
// PROTOTYPE are extensions embeddable?
// for ordinary embeddable struct types we import private members so that we can report appropriate errors if the structure is used
var isOrdinaryEmbeddableStruct = (this.TypeKind == TypeKind.Struct) && (this.SpecialType == Microsoft.CodeAnalysis.SpecialType.None) && this.ContainingAssembly.IsLinked;
var extensionMarkerMethod = TryGetExtensionMarkerMethod();
var extensionMarkerMethod = TryGetExtensionInfo().MarkerMethod;

try
{
Expand Down
35 changes: 35 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceExtensionTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
Expand All @@ -18,6 +19,8 @@ internal sealed class SourceExtensionTypeSymbol : SourceNamedTypeSymbol
private ExtensionInfo _lazyDeclaredExtensionInfo = ExtensionInfo.Sentinel;
// PROTOTYPE consider renaming ExtensionUnderlyingType->ExtendedType (here and elsewhere)
private TypeSymbol? _lazyExtensionUnderlyingType = ErrorTypeSymbol.UnknownResultType;
// For non-static extensions, we emit a field of the underlying type
private FieldSymbol? _lazyUnderlyingInstanceField = null;

internal SourceExtensionTypeSymbol(NamespaceOrTypeSymbol containingSymbol, MergedTypeDeclaration declaration, BindingDiagnosticBag diagnostics)
: base(containingSymbol, declaration, diagnostics)
Expand Down Expand Up @@ -395,5 +398,37 @@ internal static bool IsRestrictedExtensionUnderlyingType(TypeSymbol type)

return false;
}

private FieldSymbol? UnderlyingInstanceField
{
get
{
if (IsStatic)
{
throw ExceptionUtilities.Unreachable();
}

var extendedType = GetExtendedTypeNoUseSiteDiagnostics(null);
if (extendedType is null)
{
return null;
}

if (_lazyUnderlyingInstanceField is null)
{
var field = new SynthesizedFieldSymbol(this, extendedType, WellKnownMemberNames.ExtensionFieldName);
Copy link
Contributor

@AlekseyTs AlekseyTs May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

new SynthesizedFieldSymbol(this, extendedType, WellKnownMemberNames.ExtensionFieldName);

Should this field be explicitly aligned? #Closed

Copy link
Member Author

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);
            }
        }

Copy link
Contributor

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?

Copy link
Member Author

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.

Interlocked.CompareExchange(ref _lazyUnderlyingInstanceField, field, comparand: null);
}

return _lazyUnderlyingInstanceField;
}
}

internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
{
return !IsStatic && UnderlyingInstanceField is { } underlyingField
? [underlyingField, .. base.GetFieldsToEmit()]
: base.GetFieldsToEmit();
}
}
}
Loading
Loading