Add AOT annotations to System.Private.Xml and System.Data.Common #109528
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-xml |
DAM on TypeDesc would require bubbling up RDC to parts of TypeScope (AddPrimitive of byte[]) that was unnecessarily viral.
sbomer
changed the title
src/libraries/System.Data.Common/src/System/Xml/XmlBoundElement.cs
Outdated
Show resolved
Hide resolved
| @@ -429,6 +429,7 @@ internal void NewArray(Type elementType, object len) | |||
| _ilGen!.Emit(OpCodes.Newarr, elementType); | |||
| } | |||
|
|
|||
| [RequiresDynamicCode(XmlSerializer.AotSerializationWarning)] | |||
There was a problem hiding this comment.
I would think this whole class needs to be annotated with [RequiresDynamicCode].
There was a problem hiding this comment.
The only places that popped up were CreateAssemblyBuilder (because it calls DefineDynamicAssembly, and StackallocSpan (because it calls MakeGenericType).
The rest of the code in this class seems OK because it's built on abstractions like TypeBuilder that don't inherently have problems with AOT (but it won't be possible to create a type builder for a dynamic assembly).
That said, we could add annotations to the class to signal that it's not intended to be used without dynamic code support. @MichalStrehovsky curious if you have any input on this.
There was a problem hiding this comment.
Is it correct that ILGenerator isn't marked as RequiresDynamicCode?
Is the thinking that you can generate IL all you want in a native AOT application? You just can't execute it.
There was a problem hiding this comment.
Is it correct that
ILGeneratorisn't marked asRequiresDynamicCode?
AFAIK (didn't check) one could use ILGenerator with the new "Reflection.Emit SaveToDisk" APIs - those don't actually produce IL that is executable within the current process (unless one then loads the assembly from disk). ILGenerator by itself doesn't seem harmful. It's mostly about those that generate executable code, like the one obtained from a DynamicMethod.
...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs
Outdated
Show resolved
Hide resolved
| @@ -87,7 +87,6 @@ internal sealed class TypeDesc | |||
| private TypeDesc? _nullableTypeDesc; | |||
| private readonly TypeKind _kind; | |||
| private readonly XmlSchemaType? _dataType; | |||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] | |||
There was a problem hiding this comment.
Why remove these annotations? Typically we try to keep them if they "work" so if new code starts calling the methods it is annotated correctly.
There was a problem hiding this comment.
The DAMT.All on AddSoapEncodedPrimitive was producing lots of warnings for the call that passes in a byte[], like:
AOT analysis warning IL3050: System.Xml.Serialization.TypeScope.AddSoapEncodedTypes(String): Using member 'System.Array.InternalCreate(RuntimeType,Int32,Int32*,Int32*)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available.
I don't think the mentioned Array methods would be used on this code path, but I didn't feel totally comfortable suppressing them, because the type gets passed into TypeDesc that might be returned from (for example) TypeScope.GetTypeDesc - and from there it was hard to verify that the type didn't leak out.
I tried annotating TypeScope with RequiresDynamicCode instead, but this becomes more viral, so I thought it better to remove the DAMT annotations (I'm not sure what value they were adding in the first place).
There was a problem hiding this comment.
Would the newly added DAMT annotations be better for this (once I get that merged)? (I don't have an opinion about keeping the annotations or removing or updating it since I don't know what any of this is for - just bringing up one more option).
There was a problem hiding this comment.
It's possible that All wasn't necessary - but the new options still don't help with the warnings from AddSoapEncodedPrimitive. It looks like all of the code paths where the the annotations I removed were relevant were only reachable in the first place from scopes already guarded by RUC, so my preference is to remove them.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs
Outdated
Show resolved
Hide resolved
- Fix whitespace - Suppress warning for ToArray of known reference type - RequiresUnreferencedCode -> RequiresDynamicCode
Contributes to #75480