The 'seal' optimization fails with XmlSerializer

[rolfbjarne]

I'm not sure if this is a bug in the linker or the BCL, but if I enable the seal optimization:

<_ExtraTrimmerArgs>$(_ExtraTrimmerArgs) --enable-opt sealer</_ExtraTrimmerArgs>

then the xml serializer stops working with:

System.TypeLoadException: Could not load type 'Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializer1' from assembly 'Microsoft.GeneratedCode, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' because the parent type is sealed.
   at System.Reflection.Emit.TypeBuilder.TermCreateClass(QCallModule , Int32 , ObjectHandleOnStack )
   at System.Reflection.Emit.TypeBuilder.CreateTypeNoLock()
   at System.Reflection.Emit.TypeBuilder.CreateTypeInfo()
   at System.Xml.Serialization.XmlSerializationILGen.GenerateBaseSerializer(String , String , String , CodeIdentifiers )
   at System.Xml.Serialization.TempAssembly.GenerateRefEmitAssembly(XmlMapping[] , Type[] , String )
   at System.Xml.Serialization.TempAssembly..ctor(XmlMapping[] , Type[] , String , String )
   at System.Xml.Serialization.XmlSerializer.GenerateTempAssembly(XmlMapping xmlMapping, Type type, String defaultNamespace, String location)
   at System.Xml.Serialization.XmlSerializer.GenerateTempAssembly(XmlMapping xmlMapping, Type type, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type)
   at LinkSdk.Serialization.XmlSerializationTest.Response.get_Serializer() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 63
   at LinkSdk.Serialization.XmlSerializationTest.Response.Deserialize(String xml) in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 73
   at LinkSdk.Serialization.XmlSerializationTest.Bug1820_GenericList() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 188
   at App.Main() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 27

Repro:
consoleapp-18dd81d.zip

Download & extract & run "make linked notlinked" (you might have to adjust/remove NuGet.config depending on the version of .NET installed):

$ make linked notlinked
** Executing linked app
bin/Debug/net6.0/osx-x64/publish/myproject
❌ failed: System.TypeLoadException: Could not load type 'Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializer1' from assembly 'Microsoft.GeneratedCode, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' because the parent type is sealed.
   at System.Reflection.Emit.TypeBuilder.TermCreateClass(QCallModule , Int32 , ObjectHandleOnStack )
   at System.Reflection.Emit.TypeBuilder.CreateTypeNoLock()
   at System.Reflection.Emit.TypeBuilder.CreateTypeInfo()
   at System.Xml.Serialization.XmlSerializationILGen.GenerateBaseSerializer(String , String , String , CodeIdentifiers )
   at System.Xml.Serialization.TempAssembly.GenerateRefEmitAssembly(XmlMapping[] , Type[] , String )
   at System.Xml.Serialization.TempAssembly..ctor(XmlMapping[] , Type[] , String , String )
   at System.Xml.Serialization.XmlSerializer.GenerateTempAssembly(XmlMapping xmlMapping, Type type, String defaultNamespace, String location)
   at System.Xml.Serialization.XmlSerializer.GenerateTempAssembly(XmlMapping xmlMapping, Type type, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type)
   at LinkSdk.Serialization.XmlSerializationTest.Response.get_Serializer() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 63
   at LinkSdk.Serialization.XmlSerializationTest.Response.Deserialize(String xml) in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 73
   at LinkSdk.Serialization.XmlSerializationTest.Bug1820_GenericList() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 188
   at App.Main() in /Users/rolf/test/dotnet/consoleapp/Program.cs:line 27
** Executing notlinked app
bin/Debug/net6.0/osx-x64/publish/myproject
✅ succeeded: LinkSdk.Serialization.XmlSerializationTest+Response

$ dotnet --version
6.0.100-rc.2.21420.30

Binlogs:
binlogs.zip

[marek-safar]

It's very likely a bug in linker where we don't track correctly reflection-emit APIs as something that requires the base type to subclass.

@vitek-karas does the data flow analysis have such data?

[vitek-karas]

Two options I can think of:

• If type is marked as accessed via reflection - don't seal it. But this feels a bit too much.
• Marking type through DynamicallyAccessedMembers(All) also means "don't seal".

The second DAM(All) is because we use it in the TypeBuilder.DefineType - and it's possible to flow this through multiple layers correctly.

The most precise would obviously be to have a special data flow annotation for this - but it's a rather rare case, not sure it's worth the complexity.

I think the DAM(All) solution sounds reasonable.

@MichalStrehovsky , @sbomer - ideas/opinions

[MichalStrehovsky]

We don't know what types are accessed through reflection. One can call object.GetType on anything and the type becomes accessed through reflection.

DAM(All) would probably work but it's a bit of a hack - we would be making an assumption on how TypeBuilder.DefineType is annotated in the BCL and if that ever changes we might not realize.

I thought sealer optimization is unsupported whenever Reflection.Emit is allowed - it would be the safest position to take on this.

[vitek-karas]

DAM(All) would probably work but it's a bit of a hack - we would be making an assumption on how TypeBuilder.DefineType is annotated in the BCL and if that ever changes we might not realize.

I agree that it's a bit of a hack... it just felt like the least "evil". But if we think it's OK to simply disable the optimization all up, that works obviously as well.

@rolfbjarne is there a reason you were trying to enable the optimization on this app?

[rolfbjarne]

@rolfbjarne is there a reason you were trying to enable the optimization on this app?

We enable it by default in a few scenarios to minimize app size.

This is our current logic:

https://github.com/xamarin/xamarin-macios/blob/219fb1a753a3402e6c2267b437d1d095c5b7a1da/dotnet/targets/Xamarin.Shared.Sdk.targets#L483-L484

[MichalStrehovsky]

<!-- If a release build and the app is not extensible (no interpreter or JIT/code-loading for macOS) then the sealer optimization can be used -->

If illink were to make that one line work, would it fix the scenario? XmlSerializer is about to start emitting new code - if there's no interpreter/JIT like the comment says, it'll fail anyway, won't it?

(Btw, XmlSerializer does have codepaths that work without Reflection.Emit - we would just need to expose them as a feature switch: dotnet/runtimelab#593 and dotnet/runtimelab#600 should be all that's needed.)

[vitek-karas]

+1 to what @MichalStrehovsky says above. I incorrectly thought that we're using pre-generated serialization assemblies, but that's not the case.

@MichalStrehovsky would it make sense to modify the XmlSerializer to behave similarly to RegEx where it avoids Ref.Emit if the runtime states that it can't do that? Maybe we don't even need a feature switch for this then. RuntimeFeature.IsDynamicCodeCompiled - I guess it would rule out using interpreter, which might be desirable in some cases. But it's questionable if interpreter over generated IL is faster then simple reflection based serializer (would have to be measured).

[MichalStrehovsky]

That would be more of a question to the team that owns XmlSerializer. They currently drive these decisions using an enum (that captures more than just 2 values) - the feature switch I added in runtimelab hardcodes the enum to a certain value. They have the enum for things like testability - RegEx has RegexOptions.Compiled that allows exercising both options from tests but XmlSerializer doesn't have that so they test by flipping the enum using private reflection.

If we expose the "no JIT/no interpreter" option as a supported thing, there should probably be a .NET 6 issue on doing something with this. @rolfbjarne is the no JIT/no interpreter option supported officially or is it just a power user feature that we offer to close partners that know how to deal with this, but don't otherwise document?

[vitek-karas]

I thought sealer optimization is unsupported whenever Reflection.Emit is allowed - it would be the safest position to take on this.

If we agree on this we should make it a rule in the code and warn/fail. We would probably need a feature switch for this though (linker itself can hardly tell if ref.emit will work or not).

[MichalStrehovsky]

Yeah, linker is not in the position to know whether this is a safe optimization to do and whether it was paired with something that makes it okay. AFAIK the general .NET SDK doesn't expose sealing as an option at all (ability to inject arbitrary arguments into linker command line is not a supported territory).

It is the responsibility of the SDK to pair it with something that makes the optimization okay (like the Xamarin SDK does, where it enables it on iPlatforms only if interpreter got disabled).

[rolfbjarne]

(Btw, XmlSerializer does have codepaths that work without Reflection.Emit - we would just need to expose them as a feature switch: dotnet/runtimelab#593 and dotnet/runtimelab#600 should be all that's needed.)

In fact it seems that's the problem: XmlSerializer shouldn't use Reflection.Emit in this case (I believe that's supposed to be automatic after dotnet/runtime#56604, but it isn't working).

[rolfbjarne]

I've filed dotnet/runtime#59167 to track the problem with XmlSerializer, so I'm closing this one, since there doesn't seem to be any problems with the linker.