Skip to content

Use DAM binder for MarkEntireType #2206

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

Closed
wants to merge 2 commits into from
Closed

Use DAM binder for MarkEntireType #2206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0537831
Use DAM binder for MarkEntireType
sbomer Aug 11, 2021
a305c38
Undo PreserveDependency change
sbomer Aug 11, 2021
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
100 changes: 45 additions & 55 deletions src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ public partial class MarkStep : IStep
readonly List<(TypeDefinition Type, MethodBody Body, Instruction Instr)> _pending_isinst_instr;
UnreachableBlocksOptimizer _unreachableBlocksOptimizer;
MarkStepContext _markContext;
readonly HashSet<TypeDefinition> _entireTypesMarked;
DynamicallyAccessedMembersTypeHierarchy _dynamicallyAccessedMembersTypeHierarchy;
MarkScopeStack _scopeStack;

Expand All @@ -69,12 +68,8 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH

#if DEBUG
static readonly DependencyKind[] _entireTypeReasons = new DependencyKind[] {
DependencyKind.AccessedViaReflection,
DependencyKind.BaseType,
DependencyKind.MemberInAssembly,
DependencyKind.PreservedDependency,
DependencyKind.NestedType,
DependencyKind.TypeInAssembly,
DependencyKind.Unspecified,
};

static readonly DependencyKind[] _fieldReasons = new DependencyKind[] {
Expand All @@ -90,6 +85,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
DependencyKind.FieldOnGenericInstance,
DependencyKind.InteropMethodDependency,
DependencyKind.Ldtoken,
DependencyKind.MemberInAssembly,
DependencyKind.MemberOfType,
DependencyKind.DynamicDependency,
DependencyKind.ReferencedBySpecialAttribute,
Expand Down Expand Up @@ -118,12 +114,12 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
DependencyKind.GenericParameterConstraintType,
DependencyKind.InterfaceImplementationInterfaceType,
DependencyKind.Ldtoken,
DependencyKind.MemberInAssembly,
DependencyKind.ModifierType,
DependencyKind.InstructionTypeRef,
DependencyKind.ParameterType,
DependencyKind.ReferencedBySpecialAttribute,
DependencyKind.ReturnType,
DependencyKind.TypeInAssembly,
DependencyKind.UnreachableBodyRequirement,
DependencyKind.VariableType,
DependencyKind.ParameterMarshalSpec,
Expand Down Expand Up @@ -157,7 +153,7 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH
DependencyKind.Ldftn,
DependencyKind.Ldtoken,
DependencyKind.Ldvirtftn,
DependencyKind.MemberOfType,
DependencyKind.MemberInAssembly,
DependencyKind.MethodForInstantiatedType,
DependencyKind.MethodForSpecialType,
DependencyKind.MethodImplOverride,
Expand Down Expand Up @@ -194,7 +190,6 @@ public MarkStep ()
_dynamicInterfaceCastableImplementationTypes = new List<TypeDefinition> ();
_unreachableBodies = new List<(MethodBody, MarkScopeStack.Scope)> ();
_pending_isinst_instr = new List<(TypeDefinition, MethodBody, Instruction)> ();
_entireTypesMarked = new HashSet<TypeDefinition> ();
}

public AnnotationStore Annotations => _context.Annotations;
Expand Down Expand Up @@ -309,6 +304,7 @@ protected bool IsFullyPreserved (TypeDefinition type)
return false;
}

// Try to avoid calling this more than once for a type since it is not cached.
internal void MarkEntireType (TypeDefinition type, in DependencyInfo reason)
{
#if DEBUG
Expand All @@ -320,47 +316,42 @@ internal void MarkEntireType (TypeDefinition type, in DependencyInfo reason)
// In which case we would generate warnings with no source (hard to debug)
using var _ = _scopeStack.CurrentScope.Origin.MemberDefinition == null ? _scopeStack.PushScope (new MessageOrigin (type)) : null;

if (!_entireTypesMarked.Add (type))
return;

if (type.HasNestedTypes) {
foreach (TypeDefinition nested in type.NestedTypes)
MarkEntireType (nested, new DependencyInfo (DependencyKind.NestedType, type));
}

Annotations.Mark (type, reason);
MarkCustomAttributes (type, new DependencyInfo (DependencyKind.CustomAttribute, type));
MarkTypeSpecialCustomAttributes (type);

if (type.HasInterfaces) {
foreach (InterfaceImplementation iface in type.Interfaces)
MarkInterfaceImplementation (iface, new MessageOrigin (type));
// Calling MarkType here would result in extra warnings for cctors, ctors, delegate methods,
// serialization ctors, and overrides of preserved abstract methods.
// Instead, we only do extra type marking not already covered by the other cases in MarkEntireType.
void MarkTypeOnly (TypeDefinition type, in DependencyInfo reason)
Comment on lines +319 to +322
Copy link
Member

@vitek-karas vitek-karas Aug 12, 2021

Choose a reason for hiding this comment

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

This is something we should also solve one day... there are already some cases where it's very hard to prove that marking entire type does the same thing as mark type in the end. For example calling MarkEntireType(typeof(SomeGeneric<TestType>)) will not correctly mark the generic arguments (for example it doesn't apply DAM) - it's just that there are no cases where this happens currently, so it's not a problem.

Obviously for another day...

{
Annotations.Mark (type, reason);
MarkCustomAttributes (type, new DependencyInfo (DependencyKind.CustomAttribute, type));
MarkTypeSpecialCustomAttributes (type);
MarkGenericParameterProvider (type);
}

MarkGenericParameterProvider (type);
MarkTypeOnly (type, reason);
Copy link
Member

@vitek-karas vitek-karas Aug 12, 2021

Choose a reason for hiding this comment

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

This comment is about the previous change in this area:
We don't do anything with base type here - it's the behavior we have for copy assemblies, so that's fine, but I think it's worth adding a comment - reading the code it looks really weird.

Side note: the base type will end up being marked in pretty much all cases anyway, since we call MarkMethod which will in turn call MarkType on the declaring type, which will mark the base type as appropriate.


if (type.HasFields) {
foreach (FieldDefinition field in type.Fields) {
MarkField (field, new DependencyInfo (DependencyKind.MemberOfType, type));
}
}

if (type.HasMethods) {
foreach (MethodDefinition method in type.Methods) {
// Note: This actually has nothing to do with dynamically accessed members - here we just use the DynamicallyAccessedMemberTypes.All
// as a convenient way to select every member of the type.
foreach (var member in type.GetDynamicallyAccessedMembers (_context, DynamicallyAccessedMemberTypes.All, declaredOnly: true)) {
switch (member) {
case MethodDefinition method:
Annotations.SetAction (method, MethodAction.ForceParse);
MarkMethod (method, new DependencyInfo (DependencyKind.MemberOfType, type));
}
}

if (type.HasProperties) {
foreach (var property in type.Properties) {
MarkProperty (property, new DependencyInfo (DependencyKind.MemberOfType, type));
}
}

if (type.HasEvents) {
foreach (var ev in type.Events) {
MarkEvent (ev, new DependencyInfo (DependencyKind.MemberOfType, type));
MarkMethod (method, reason);
break;
case FieldDefinition field:
MarkField (field, reason);
break;
case TypeDefinition nestedType:
MarkTypeOnly (nestedType, reason);
break;
case PropertyDefinition property:
MarkProperty (property, reason);
break;
case EventDefinition @event:
MarkEvent (@event, reason);
break;
case InterfaceImplementation iface:
MarkInterfaceImplementation (iface, new MessageOrigin (type));
break;
Comment on lines +352 to +354
Copy link
Member

@vitek-karas vitek-karas Aug 12, 2021

Choose a reason for hiding this comment

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

If I read the code in GetAllOnType correctly, it will not return any interfaces when declaredOnly:true.

Means we're probably missing a test as well.

}
}
}
Expand Down Expand Up @@ -1387,7 +1378,7 @@ void MarkEntireAssembly (AssemblyDefinition assembly)
MarkCustomAttributes (module, new DependencyInfo (DependencyKind.AssemblyOrModuleAttribute, module));

foreach (TypeDefinition type in module.Types)
MarkEntireType (type, new DependencyInfo (DependencyKind.TypeInAssembly, assembly));
MarkEntireType (type, new DependencyInfo (DependencyKind.MemberInAssembly, assembly));

foreach (ExportedType exportedType in module.ExportedTypes) {
MarkingHelpers.MarkExportedType (exportedType, module, new DependencyInfo (DependencyKind.ExportedType, assembly));
Expand All @@ -1403,7 +1394,7 @@ void ProcessModuleType (AssemblyDefinition assembly)
// The <Module> type may have an initializer, in which case we want to keep it.
TypeDefinition moduleType = assembly.MainModule.Types.FirstOrDefault (t => t.MetadataToken.RID == 1);
if (moduleType != null && moduleType.HasMethods)
MarkType (moduleType, new DependencyInfo (DependencyKind.TypeInAssembly, assembly));
MarkType (moduleType, new DependencyInfo (DependencyKind.MemberInAssembly, assembly));
}

bool ProcessLazyAttributes ()
Expand Down Expand Up @@ -2803,13 +2794,12 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin
case DependencyKind.MethodForInstantiatedType:
case DependencyKind.VirtualNeededDueToPreservedScope:

// Used when marked because the member must be kept for the type to function (for example explicit layout,
// or because the type is included as a whole for some other reasons). This alone should not act as a base
// for raising a warning.
// Note that "include whole type" due to dynamic access is handled specifically in MarkEntireType
// and the DependencyKind in that case will be one of the dynamic acccess kinds and not MemberOfType
// since in those cases the warnings are desirable (potential access through reflection).
case DependencyKind.MemberOfType:
// Used when the type is included as a whole due to the assembly action. This alone should not act as a
// base for raising a warning.
// Note that "include whole type" due to dynamic access is handled differently and the DependencyKind in
// that case will be one of the dynamic access kinds and not MemberOfType since in those cases the warnings
// are desirable (potential access through reflection).
case DependencyKind.MemberInAssembly:

// We should not be generating code which would produce warnings
case DependencyKind.UnreachableBodyRequirement:
Expand Down
3 changes: 1 addition & 2 deletions src/linker/Linker/DependencyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public enum DependencyKind
AssemblyOrModuleAttribute = 4, // assembly/module -> entry attribute

// Membership and containment relationships
NestedType = 5, // parent type -> nested type
MemberOfType = 6, // type -> member
DeclaringType = 7, // member -> type
FieldOnGenericInstance = 8, // fieldref on instantiated generic -> field on generic typedef
Expand Down Expand Up @@ -52,7 +51,7 @@ public enum DependencyKind
// Modules and assemblies
ScopeOfType = 28, // type -> module/assembly
AssemblyOfModule = 81, // module -> assembly
TypeInAssembly = 29, // assembly -> type
MemberInAssembly = 29, // assembly -> member
ModuleOfExportedType = 30, // exported type -> module
ExportedType = 31, // type -> exported type

Expand Down