AssemblyBuilder.Save add custom attributes handling.

[buyaa-n]

Contributes to #62956
This PR include following updates:

• Expose internal ConstructorInfo Ctor, byte[] Data properties from CustomAttributeBuilder and use them for SetCustomAttribute(CustomAttributeBuilder customBuilder) in shared *Builder types. (initially was planning to make the fields internal, but Mono using properties).
• Remove protected override void SetCustomAttributeCore(CustomAttributeBuilder customBuilder) implementations and convert byte[] binaryAttribute parameter of SetCustomAttributeCore(ConstructorInfo con, byte[] binaryAttribute) into ReadOnlySpan<byte> binaryAttribute for all runtime *Builder types
• Add CustomAttribute handling for new AssemblyBuilder implementation and corresponding tests
• Add Pseudo custom attributes handling and testing

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR include following updates:

• Expose internal ConstructorInfo Ctor, byte[] Data properties from CustomAttributeBuilder and use them for SetCustomAttribute(CustomAttributeBuilder customBuilder) in shared *Builder types. (initially was planning to make the fields internal, but Mono using properties).
• Remove protected override void SetCustomAttributeCore(CustomAttributeBuilder customBuilder) implementations from runtime *Builder types
• Add CustomAttribute handling for new AssemblyBuilder implementation and corresponding tests
• Pseudo custom attributes handling is not complete, so creating a draft PR

Author:	buyaa-n
Assignees:	buyaa-n
Labels:	area-System.Reflection, new-api-needs-documentation
Milestone:	-

[jkotas]

This is introducing an extra array copy. Do you remember whether this regression was considered during the API review discussion that proposed changing the argument type to ReadOnlySpan?

[buyaa-n]

Do you remember whether this regression was considered during the API review discussion that proposed changing the argument type to ReadOnlySpan?

No, I don't think so, there were not much discussion about this change, quickly agreed for this update. Would you suggest to keep the SetCustomAttributeCore(ConstructorInfo con, byte[] binaryAttribute) for now?

[jkotas]

I think that the extra copy is fine, it is just something that I have noticed and that I did not expect. I do not have a strong opinion about it. What do you think?

[buyaa-n]

I think this path would be used rarely (attributes for generic type parameter) so probably fine, but for the new assembly builder implementation we are creating this copy for each attribute:

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/CustomAttributeWrapper.cs

Line 16 in 02536ba

_binaryAttribute = binaryAttribute.ToArray();

I think I can workaround that to keep the BlobHandle of the ReadOnlySpan<byte> instead of byte[] in CustomAttributeWrapper. But that needs #81059 merged and a public API for ReadOnlySpan<byte> overload is added

[buyaa-n]

Added TODO for using BlobHandle instead of byte[].

There is similar allocation added in mono:

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

Line 85 in a7ed5ce

data = binaryAttribute.ToArray();

It is OK in case the SetCustomAttribute(ConstructorInfo con, byte[] binaryAttribute) overload used directly as previously it was also cloning the byte array

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

Line 86 in 2087ceb

data = (byte[])binaryAttribute.Clone();

but as you know if the SetCustomAttribute(CustomAttributeBuilder customBuilder) overload is used it will call SetCustomAttributeCore(customBuilder.Ctor, customBuilder.Data) and going to create another copy ...

[jkotas]

Thanks

[buyaa-n]

Thank you for review!

Failures unrelated and Build Analysis shows that all are known, merging.