Replace linker descriptor for AssemblyBuilder with DynamicDependency #38710

marek-safar · 2020-07-02T14:08:00Z

for better linking when AssemblyBuilder is not instantiated.

Contributes to #38692

for better linking when AssemblyBuilder is not instantiated. Contributes to dotnet#38692

Dotnet-GitSync-Bot · 2020-07-02T14:08:03Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

stephentoub · 2020-07-02T15:07:26Z

src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

+        [DynamicDependency("machine")]
+        [DynamicDependency("corlib_internal")]
+        [DynamicDependency("type_forwarders")]
+        [DynamicDependency("pktoken")]


Nit: are you able to use nameof in any of these?

Could use a comment as to why the dependencies are on this method.

yeah, will switch to nameof

vargaz · 2020-07-02T15:19:40Z

src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

@@ -202,13 +197,12 @@ public sealed class AssemblyBuilder : Assembly
        private object? permissions_minimum;
        private object? permissions_optional;
        private object? permissions_refused;
-        private PortableExecutableKinds peKind;
-        private ImageFileMachine machine;
+        private int peKind;


Is this needed ?

the field is not used and it drops additional dependencies

eerhardt · 2020-07-02T15:34:52Z

I tried using this change locally (with local fixes for #38693 and #38678). However, the ILLinker still isn't able to trim the fields from AssemblyBuilder. @marek-safar pointed out offline that it is because AssemblyBuilder has a StructLayout attribute.

runtime/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

Lines 170 to 171 in a92f4f0

    
           [StructLayout(LayoutKind.Sequential)] 
        
           public sealed class AssemblyBuilder : Assembly

Even after changing that, there are other edges into Ref.Emit that are preserving most of the types (because of the other preserve=fields on other types):

TypeDef:System.Reflection.Emit.DynamicMethod size: 770
| Method:System.Delegate System.Delegate::CreateDelegate(System.Type,System.Object,System.Reflection.MethodInfo,System.Boolean,System.Boolean) [2 deps]
| Method:System.Delegate System.Delegate::CreateDelegate(System.Type,System.Reflection.MethodInfo,System.Boolean) [1 deps]
| Method:System.Delegate System.Delegate::CreateDelegate(System.Type,System.Reflection.MethodInfo) [3 deps]
| Method:System.Void Microsoft.AspNetCore.Components.Reflection.MemberAssignment/PropertySetter`2::.ctor(System.Reflection.MethodInfo,System.Boolean) [2 deps]

and

 TypeDef:System.Reflection.Emit.ModuleBuilder size: 423
| Field:System.Reflection.Emit.ModuleBuilder System.Reflection.Emit.TypeBuilder::pmodule [1 deps]
| TypeDef:System.Reflection.Emit.TypeBuilder [72 deps]
| Method:System.Object System.Activator::CreateInstance(System.Type,System.Reflection.BindingFlags,System.Reflection.Binder,System.Object[],System.Globalization.CultureInfo,System.Object[]) [3 deps]
| Method:System.Object System.Activator::CreateInstance(System.Type,System.Object[]) [5 deps]
| Method:Microsoft.AspNetCore.Components.Reflection.IPropertySetter Microsoft.AspNetCore.Components.Reflection.MemberAssignment::CreatePropertySetter(System.Type,System.Reflection.PropertyInfo,System.Boolean) [3 deps]

MichalStrehovsky · 2020-07-03T09:45:31Z

I think this should be doable without any annotations.

StructLayout.Sequential keeps all fields on the type, for good reasons.

But linker is too eager to respect the layout. For reference types (and only reference types), the layout only becomes relevant once you allocate an instance. Linker should defer marking the fields until it sees the type as instantiated (i.e. constructor called).

Once linker does that, the annotation will be unnecessary and this will Just Work™.

MichalStrehovsky · 2020-07-03T11:50:06Z

I think this should be doable without any annotations.

Linker change: dotnet/linker#1309

MichalStrehovsky · 2020-07-03T12:24:36Z

src/mono/netcore/System.Private.CoreLib/src/System/Reflection/Emit/AssemblyBuilder.Mono.cs

@@ -224,7 +218,8 @@ public sealed class AssemblyBuilder : Assembly
        [MethodImplAttribute(MethodImplOptions.InternalCall)]
        private static extern void UpdateNativeCustomAttributes(AssemblyBuilder ab);

-        internal AssemblyBuilder(AssemblyName n, string? directory, AssemblyBuilderAccess access, bool corlib_internal)
+        [DynamicDependency(nameof(pktoken))] // Automatically keeps all previous fields too due to StructLayout


Why do we need DynamicDependency at all? The class is marked StructLayout.Sequential and this annotation is on a constructor - linker is going to keep all the fields anyway if it sees the constructor is needed.

Technically we don't right now but I wanted to make this dependency more explicit. We could also fix linker to keep only used fields for struct layout classes.

I'm not sure I understand how an explicit DynamicDependency on what appears to be an arbitrary field achieves that - I think we'll find that part quite confusing when we look at this code again 5 years from now.

If we want to be explicit, can we instead use the attribute that says to keep all fields?

Replace linker descriptor for AssemblyBuilder with DynamicDependency

56fa672

for better linking when AssemblyBuilder is not instantiated. Contributes to dotnet#38692

marek-safar requested review from akoeplinger, EgorBo, steveisok and vargaz as code owners July 2, 2020 14:08

marek-safar added the area-CoreLib-mono label Jul 2, 2020

stephentoub reviewed Jul 2, 2020

View reviewed changes

stephentoub approved these changes Jul 2, 2020

View reviewed changes

vargaz reviewed Jul 2, 2020

View reviewed changes

vargaz approved these changes Jul 2, 2020

View reviewed changes

Review feedback

8c143c2

marek-safar force-pushed the subst branch from 4de4273 to 8c143c2 Compare July 3, 2020 12:21

MichalStrehovsky reviewed Jul 3, 2020

View reviewed changes

marek-safar merged commit 054fb2e into dotnet:master Jul 3, 2020

marek-safar deleted the subst branch July 3, 2020 15:11

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace linker descriptor for AssemblyBuilder with DynamicDependency #38710

Replace linker descriptor for AssemblyBuilder with DynamicDependency #38710

marek-safar commented Jul 2, 2020

Dotnet-GitSync-Bot commented Jul 2, 2020

stephentoub Jul 2, 2020

vargaz Jul 2, 2020 •

edited

Loading

marek-safar Jul 3, 2020

vargaz Jul 2, 2020

marek-safar Jul 3, 2020

eerhardt commented Jul 2, 2020

MichalStrehovsky commented Jul 3, 2020

MichalStrehovsky commented Jul 3, 2020

MichalStrehovsky Jul 3, 2020

marek-safar Jul 3, 2020

MichalStrehovsky Jul 3, 2020

stephentoub Jul 3, 2020

Replace linker descriptor for AssemblyBuilder with DynamicDependency #38710

Replace linker descriptor for AssemblyBuilder with DynamicDependency #38710

Conversation

marek-safar commented Jul 2, 2020

Dotnet-GitSync-Bot commented Jul 2, 2020

Choose a reason for hiding this comment

vargaz Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Jul 2, 2020

MichalStrehovsky commented Jul 3, 2020

MichalStrehovsky commented Jul 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vargaz Jul 2, 2020 •

edited

Loading