Skip to content

Add support for annotations in classes/interfaces/structs in the linker #1929

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

Merged
merged 32 commits into from
Apr 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
606b8e6
Flow first take
LakshanF Mar 17, 2021
6bcf85d
Use xml link attributes to test DynamicallyAccessedMembers in a type
Mar 18, 2021
e4636a1
Merge pull request #1 from tlakollo/TrimAttrSupportFlow
LakshanF Mar 18, 2021
81163aa
added missing public API to get the type annotation
LakshanF Mar 18, 2021
2b00495
Add support for StaticType in Value Node
Mar 19, 2021
5ddb0a7
Change some messages
Mar 19, 2021
1ab3e62
Need a way to ask about the type membertypes
Mar 20, 2021
bdf6f43
Fix formatting
Mar 20, 2021
88ab63f
Fix Value Node
Mar 20, 2021
990272b
Improve object.GetType
vitek-karas Mar 22, 2021
857b205
Merge pull request #2 from tlakollo/TrimAttrSupportFlow
LakshanF Mar 22, 2021
f7a5c4b
Fix the way parameters are derived from methods
vitek-karas Mar 22, 2021
39e0891
Merge pull request #3 from vitek-karas/TrimAttrSupportFlow
LakshanF Mar 22, 2021
fef4bc3
Implement marking
vitek-karas Mar 23, 2021
e5122b1
Added partial test scenarios. The test will not ocmpile since the lib…
LakshanF Mar 24, 2021
abbae0c
Fixed some typos, still the test fails and more scenarios needed to b…
LakshanF Mar 24, 2021
26f423b
Merge remote-tracking branch 'lakshanf/TrimAttrSupportFlow' into Trim…
vitek-karas Mar 25, 2021
2f8b0eb
Merge pull request #4 from vitek-karas/TrimAttrSupportFlow
LakshanF Mar 25, 2021
669d3de
Merge branch 'TrimAttrSupportFlow' of https://github.com/LakshanF/lin…
vitek-karas Mar 25, 2021
fec67b1
Fixes build, tests and ValueNode equality
vitek-karas Mar 26, 2021
5786b31
Formatting
vitek-karas Mar 26, 2021
e3cddfe
Merge pull request #5 from vitek-karas/TrimAttrSupportFlow
tlakollo Mar 29, 2021
5fbfb2a
Merge remote-tracking branch 'origin/main' into TrimAttrSupportFlow
vitek-karas Mar 30, 2021
4c4473c
Use actual attributes in tests
vitek-karas Mar 30, 2021
bda9ac5
Add more tests and cleanup
vitek-karas Mar 31, 2021
1e6a54c
Merge pull request #6 from vitek-karas/TrimAttrSupportFlow
vitek-karas Mar 31, 2021
fd491fa
Merge branch 'main' of https://github.com/LakshanF/linker into TrimAt…
Apr 1, 2021
69e06bf
Merge branch 'TrimAttrSupportFlow' of https://github.com/LakshanF/lin…
Apr 1, 2021
eadda15
PR Feedback
vitek-karas Apr 6, 2021
901b1af
Merge branch 'TrimAttrSupportFlow' of https://github.com/LakshanF/lin…
Apr 9, 2021
274ed43
Fix some spelling
Apr 9, 2021
2980f6e
Change TODOs to issue references
vitek-karas Apr 9, 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
198 changes: 198 additions & 0 deletions src/linker/Linker.Dataflow/DynamicallyAccessedMembersTypeHierarchy.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using Mono.Cecil;
using Mono.Linker.Steps;

namespace Mono.Linker.Dataflow
{
class DynamicallyAccessedMembersTypeHierarchy
Copy link
Member

Choose a reason for hiding this comment

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

Naming: I would welcome any suggestion for a better name...

{
readonly LinkContext _context;
readonly MarkStep _markStep;

// Cache of DynamicallyAccessedMembers annotations applied to types and their hierarchies
// Values
// annotation - the aggregated annotation value from the entire base and interface hierarchy of the given type
// If the type has a base class with annotation a1 and an interface with annotation a2, the stored
// annotation is a1 | a2.
// applied - set to true once the annotation was applied to the type
// This only happens once the right reflection pattern is found.
// If a new type is being marked and one of its base types/interface has the applied set to true
// the new type will apply its annotation and will also set its applied to true.
// Non-interface types
// - Only marked types with non-empty annotation are put into the cache
// - Non-marked types are not stored in the cache
// - Marked types which are not in the cache don't have any annotation
// Interface types
// - All interface types accessible from marked types are stored in the cache
// - If the interface type doesn't have annotation the value None is stored here
//
// It's not possible to use the marking as a filter for interfaces in the cache
// because interfaces are marked late and in effectively random order.
// For this cache to be effective we need to be able to fill it for all base types and interfaces
// of a type which is currently being marked - at which point the interfaces are not yet marked.
readonly Dictionary<TypeDefinition, (DynamicallyAccessedMemberTypes annotation, bool applied)> _typesInDynamicallyAccessedMembersHierarchy;
Copy link
Member

Choose a reason for hiding this comment

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

This cache will include all interfaces reachable from marked types - which can be relatively large number (1000s). It is built regardless of the annotations being used or even present, since it acts as a negative cache as well.

Alternatively this could be implemented by re-computing the annotation for interface each type it's needed - basically trade memory or CPU. I went with the cache as some kind of cache would still be needed for the applied bit, and the memory impact doesn't seem that bad (couple of KB).


public DynamicallyAccessedMembersTypeHierarchy (LinkContext context, MarkStep markStep)
{
_context = context;
_markStep = markStep;
_typesInDynamicallyAccessedMembersHierarchy = new Dictionary<TypeDefinition, (DynamicallyAccessedMemberTypes, bool)> ();
}

public (DynamicallyAccessedMemberTypes annotation, bool applied) ProcessMarkedTypeForDynamicallyAccessedMembersHierarchy (TypeDefinition type)
{
// Non-interfaces must be marked already
Debug.Assert (type.IsInterface || _context.Annotations.IsMarked (type));

DynamicallyAccessedMemberTypes annotation = _context.Annotations.FlowAnnotations.GetTypeAnnotation (type);
bool apply = false;

// We'll use the cache also as a way to detect and avoid recursion
// There's no possiblity to have recursion among base types, so only do this for interfaces
if (type.IsInterface) {
if (_typesInDynamicallyAccessedMembersHierarchy.TryGetValue (type, out var existingValue))
return existingValue;

_typesInDynamicallyAccessedMembersHierarchy.Add (type, (annotation, false));
}

// Base should already be marked (since we're marking its derived type now)
// so we should already have its cached values filled.
TypeDefinition baseType = type.BaseType?.Resolve ();
Debug.Assert (baseType == null || _context.Annotations.IsMarked (baseType));
if (baseType != null && _typesInDynamicallyAccessedMembersHierarchy.TryGetValue (baseType, out var baseValue)) {
annotation |= baseValue.annotation;
apply |= baseValue.applied;
}

// For the purposes of the DynamicallyAccessedMembers type hierarchies
// we consider interfaces of marked types to be also "marked" in that
// their annotations will be applied to the type regardless if later on
// we decide to remove the interface. This is to keep the complexity of the implementation
// relatively low. In the future it could be possibly optimized.
if (type.HasInterfaces) {
foreach (InterfaceImplementation iface in type.Interfaces) {
var interfaceType = iface.InterfaceType.Resolve ();
if (interfaceType != null) {
var interfaceValue = ProcessMarkedTypeForDynamicallyAccessedMembersHierarchy (interfaceType);
annotation |= interfaceValue.annotation;
apply |= interfaceValue.applied;
}
}
}

Debug.Assert (!apply || annotation != DynamicallyAccessedMemberTypes.None);

if (apply) {
// One of the base/interface types is already marked as having the annotation applied
// so we need to apply the annotation to this type as well
var reflectionMethodBodyScanner = new ReflectionMethodBodyScanner (_context, _markStep);
var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);
using var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);

Copy link
Member

Choose a reason for hiding this comment

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

It's actually not possible to use using since we're passing it as ref (and the compiler checks that the values in using is immutable).
But it's a valid concern that we're not calling Dispose even though it's just a debug check. I modified all call sites to call Dispose - without the try/finally as it doesn't seem necessary - But I can be convinced to add that as well.

reflectionMethodBodyScanner.ApplyDynamicallyAccessedMembersToType (ref reflectionPatternContext, type, annotation);
reflectionPatternContext.Dispose ();
}

// Store the results in the cache
// Don't store empty annotations for non-interface types - we can use the presence of the row
// in the cache as indication of it instead.
// This doesn't work for interfaces, since we can't rely on them being marked (and thus have the cache
// already filled), so we need to always store the row (even if empty) for interfaces.
if (annotation != DynamicallyAccessedMemberTypes.None || type.IsInterface) {
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, apply);
}

return (annotation, apply);
}

public DynamicallyAccessedMemberTypes ApplyDynamicallyAccessedMembersToTypeHierarchy (
ReflectionMethodBodyScanner reflectionMethodBodyScanner,
ref ReflectionPatternContext reflectionPatternContext,
TypeDefinition type)
{
Debug.Assert (_context.Annotations.IsMarked (type));

// The type should be in our cache already
(var annotation, var applied) = GetCachedInfoForTypeInHierarchy (type);

// If the annotation was already applied to this type, there's no reason to repeat the operation, the result will
// be no change.
if (applied || annotation == DynamicallyAccessedMemberTypes.None)
return annotation;

// Apply the effective annotation for the type
reflectionMethodBodyScanner.ApplyDynamicallyAccessedMembersToType (ref reflectionPatternContext, type, annotation);

// Mark it as applied in the cache
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, true);

// Propagate the newly applied annotation to all derived/implementation types
// Since we don't have a data structure which would allow us to enumerate all derived/implementation types
// walk all of the types in the cache. These are good candidates as types not in the cache don't apply.
foreach (var candidate in _typesInDynamicallyAccessedMembersHierarchy) {
if (candidate.Value.annotation == DynamicallyAccessedMemberTypes.None)
continue;

ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMethodBodyScanner, ref reflectionPatternContext, candidate.Key);
}

return annotation;
}

bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
ReflectionMethodBodyScanner reflectionMethodBodyScanner,
ref ReflectionPatternContext reflectionPatternContext,
TypeDefinition type)
{
(var annotation, var applied) = GetCachedInfoForTypeInHierarchy (type);

if (annotation == DynamicallyAccessedMemberTypes.None)
return false;

if (applied)
return true;

TypeDefinition baseType = type.BaseType?.Resolve ();
if (baseType != null)
applied = ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMethodBodyScanner, ref reflectionPatternContext, baseType);

if (!applied && type.HasInterfaces) {
foreach (InterfaceImplementation iface in type.Interfaces) {
var interfaceType = iface.InterfaceType.Resolve ();
if (interfaceType != null) {
if (ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMethodBodyScanner, ref reflectionPatternContext, interfaceType)) {
applied = true;
break;
}
}
}
}

if (applied) {
reflectionMethodBodyScanner.ApplyDynamicallyAccessedMembersToType (ref reflectionPatternContext, type, annotation);
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, true);
}

return applied;
}

(DynamicallyAccessedMemberTypes annotation, bool applied) GetCachedInfoForTypeInHierarchy (TypeDefinition type)
{
Debug.Assert (type.IsInterface || _context.Annotations.IsMarked (type));

// The type should be in our cache already
if (!_typesInDynamicallyAccessedMembersHierarchy.TryGetValue (type, out var existingValue)) {
// If it's not in the cache it should be a non-interface type in which case it means there were no annotations
Debug.Assert (!type.IsInterface);
return (DynamicallyAccessedMemberTypes.None, false);
}

return existingValue;
}
}
}
18 changes: 15 additions & 3 deletions src/linker/Linker.Dataflow/FlowAnnotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field)
return DynamicallyAccessedMemberTypes.None;
}

public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type)
{
return GetAnnotations (type).TypeAnnotation;
}

public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericParameter genericParameter)
{
TypeDefinition declaringType = genericParameter.DeclaringType?.Resolve ();
Expand Down Expand Up @@ -122,6 +127,9 @@ DynamicallyAccessedMemberTypes GetMemberTypesForDynamicallyAccessedMembersAttrib

TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
{
// class, interface, struct can have annotations
DynamicallyAccessedMemberTypes typeAnnotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (type);

var annotatedFields = new ArrayBuilder<FieldAnnotation> ();

// First go over all fields with an explicit annotation
Expand Down Expand Up @@ -338,7 +346,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
}
}

return new TypeAnnotations (type, annotatedMethods.ToArray (), annotatedFields.ToArray (), typeGenericParameterAnnotations);
return new TypeAnnotations (type, typeAnnotation, annotatedMethods.ToArray (), annotatedFields.ToArray (), typeGenericParameterAnnotations);
}

static bool ScanMethodBodyForFieldAccess (MethodBody body, bool write, out FieldDefinition found)
Expand Down Expand Up @@ -518,17 +526,21 @@ void LogValidationWarning (IMetadataTokenProvider provider, IMetadataTokenProvid
readonly struct TypeAnnotations
{
readonly TypeDefinition _type;
readonly DynamicallyAccessedMemberTypes _typeAnnotation;
readonly MethodAnnotations[] _annotatedMethods;
readonly FieldAnnotation[] _annotatedFields;
readonly DynamicallyAccessedMemberTypes[] _genericParameterAnnotations;

public TypeAnnotations (
TypeDefinition type,
DynamicallyAccessedMemberTypes typeAnnotation,
MethodAnnotations[] annotatedMethods,
FieldAnnotation[] annotatedFields,
DynamicallyAccessedMemberTypes[] genericParameterAnnotations)
=> (_type, _annotatedMethods, _annotatedFields, _genericParameterAnnotations)
= (type, annotatedMethods, annotatedFields, genericParameterAnnotations);
=> (_type, _typeAnnotation, _annotatedMethods, _annotatedFields, _genericParameterAnnotations)
= (type, typeAnnotation, annotatedMethods, annotatedFields, genericParameterAnnotations);

public DynamicallyAccessedMemberTypes TypeAnnotation { get => _typeAnnotation; }

public bool TryGetAnnotation (MethodDefinition method, out MethodAnnotations annotations)
{
Expand Down
74 changes: 47 additions & 27 deletions src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public void ScanAndProcessReturnValue (MethodBody methodBody)
var reflectionContext = new ReflectionPatternContext (_context, ShouldEnableReflectionPatternReporting (method), method, method.MethodReturnType);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, requiredMemberTypes, MethodReturnValue, method.MethodReturnType);
reflectionContext.Dispose ();
}
}
}
Expand All @@ -92,6 +93,7 @@ public void ProcessAttributeDataflow (IMemberDefinition source, MethodDefinition
var reflectionContext = new ReflectionPatternContext (_context, true, source, methodParameter);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, methodParameter);
reflectionContext.Dispose ();
}
}
}
Expand All @@ -105,6 +107,15 @@ public void ProcessAttributeDataflow (IMemberDefinition source, FieldDefinition
var reflectionContext = new ReflectionPatternContext (_context, true, source, field);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, field);
reflectionContext.Dispose ();
}

public void ApplyDynamicallyAccessedMembersToType (ref ReflectionPatternContext reflectionPatternContext, TypeDefinition type, DynamicallyAccessedMemberTypes annotation)
{
Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);

reflectionPatternContext.AnalyzingPattern ();
MarkTypeForDynamicallyAccessedMembers (ref reflectionPatternContext, type, annotation);
}

static ValueNode GetValueNodeForCustomAttributeArgument (CustomAttributeArgument argument)
Expand Down Expand Up @@ -138,6 +149,7 @@ public void ProcessGenericArgumentDataFlow (GenericParameter genericParameter, T
var reflectionContext = new ReflectionPatternContext (_context, enableReflectionPatternReporting, source, genericParameter);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, genericParameter);
reflectionContext.Dispose ();
}

ValueNode GetTypeValueNodeFromGenericArgument (TypeReference genericArgument)
Expand Down Expand Up @@ -173,7 +185,7 @@ protected override void WarnAboutInvalidILInMethod (MethodBody method, int ilOff
protected override ValueNode GetMethodParameterValue (MethodDefinition method, int parameterIndex)
{
DynamicallyAccessedMemberTypes memberTypes = _context.Annotations.FlowAnnotations.GetParameterAnnotation (method, parameterIndex);
return new MethodParameterValue (parameterIndex, memberTypes, DiagnosticUtilities.GetMethodParameterFromIndex (method, parameterIndex));
return new MethodParameterValue (method, parameterIndex, memberTypes, DiagnosticUtilities.GetMethodParameterFromIndex (method, parameterIndex));
}

protected override ValueNode GetFieldValue (MethodDefinition method, FieldDefinition field)
Expand All @@ -200,6 +212,7 @@ protected override void HandleStoreField (MethodDefinition method, FieldDefiniti
var reflectionContext = new ReflectionPatternContext (_context, ShouldEnableReflectionPatternReporting (method), method, field, operation);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, requiredMemberTypes, valueToStore, field);
reflectionContext.Dispose ();
}
}

Expand All @@ -211,6 +224,7 @@ protected override void HandleStoreParameter (MethodDefinition method, int index
var reflectionContext = new ReflectionPatternContext (_context, ShouldEnableReflectionPatternReporting (method), method, parameter, operation);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, requiredMemberTypes, valueToStore, parameter);
reflectionContext.Dispose ();
}
}

Expand Down Expand Up @@ -910,36 +924,42 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
// GetType()
//
case IntrinsicId.Object_GetType: {
// We could do better here if we start tracking the static types of values within the method body.
// Right now, this can only analyze a couple cases for which we have static information for.
TypeDefinition staticType = null;
if (methodParams[0] is MethodParameterValue methodParam) {
if (callingMethodDefinition.HasThis) {
if (methodParam.ParameterIndex == 0) {
staticType = callingMethodDefinition.DeclaringType;
} else {
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex - 1].ParameterType.ResolveToMainTypeDefinition ();
}
} else {
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex].ParameterType.ResolveToMainTypeDefinition ();
}
} else if (methodParams[0] is LoadFieldValue loadedField) {
staticType = loadedField.Field.FieldType.ResolveToMainTypeDefinition ();
}

if (staticType != null) {
// We can only analyze the Object.GetType call with the precise type if the type is sealed.
// The type could be a descendant of the type in question, making us miss reflection.
bool canUse = staticType.IsSealed;
foreach (var valueNode in methodParams[0].UniqueValues ()) {
TypeDefinition staticType = valueNode.StaticType;
if (staticType is null) {
// We don�t know anything about the type GetType was called on. Track this as a usual �result of a method call without any annotations�
methodReturnValue = MergePointValue.MergeValues (methodReturnValue, new MethodReturnValue (calledMethod.MethodReturnType, DynamicallyAccessedMemberTypes.None));
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate")) {
// We can treat this one the same as if it was a typeof() expression

if (!canUse) {
// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
// on delegates anyway so reflection on something this approximation would miss is actually safe.
canUse = staticType.IsTypeOf ("System", "Delegate");
}

if (canUse) {
methodReturnValue = new SystemTypeValue (staticType);
// We ignore the fact that the type can be annotated (see below for handling of annotated types)
// This means the annotations (if any) won't be applied - instead we rely on the exact knowledge
// of the type. So for example even if the type is annotated with PublicMethods
// but the code calls GetProperties on it - it will work - mark properties, don't mark methods
// since we ignored the fact that it's annotated.
// This can be seen a little bit as a violation of the annotation, but we already have similar cases
// where a parameter is annotated and if something in the method sets a specific known type to it
// we will also make it just work, even if the annotation doesn't match the usage.
methodReturnValue = MergePointValue.MergeValues (methodReturnValue, new SystemTypeValue (staticType));
} else {
reflectionContext.AnalyzingPattern ();

// Make sure the type is marked (this will mark it as used via reflection, which is sort of true)
// This should already be true for most cases (method params, fields, ...), but just in case
MarkType (ref reflectionContext, staticType);

var annotation = _markStep.DynamicallyAccessedMembersTypeHierarchy
.ApplyDynamicallyAccessedMembersToTypeHierarchy (this, ref reflectionContext, staticType);

reflectionContext.RecordHandledPattern ();

// Return a value which is "unknown type" with annotation. For now we'll use the return value node
// for the method, which means we're loosing the information about which staticType this
// started with. For now we don't need it, but we can add it later on.
methodReturnValue = MergePointValue.MergeValues (methodReturnValue, new MethodReturnValue (calledMethod.MethodReturnType, annotation));
}
}
}
Expand Down
Loading