Resolve ILLink warnings in System.Security.Cryptography.X509Certificates #47345
Conversation
eerhardt
requested review from
stephentoub,
bartonjs,
vitek-karas and
joperezr
| [DynamicDependency("#ctor(System.Net.Http.HttpMessageHandler)", "System.Net.Http.HttpClient", "System.Net.Http")] | ||
| [DynamicDependency("#ctor", "System.Net.Http.SocketsHttpHandler", "System.Net.Http")] | ||
| [DynamicDependency("#ctor", "System.Net.Http.HttpRequestMessage", "System.Net.Http")] | ||
| [DynamicDependency("set_PooledConnectionIdleTimeout", "System.Net.Http.SocketsHttpHandler", "System.Net.Http")] | ||
| [DynamicDependency("RequestUri", "System.Net.Http.HttpRequestMessage", "System.Net.Http")] | ||
| [DynamicDependency("Send", "System.Net.Http.HttpClient", "System.Net.Http")] | ||
| [DynamicDependency("Content", "System.Net.Http.HttpResponseMessage", "System.Net.Http")] | ||
| [DynamicDependency("ReadAsStream", "System.Net.Http.HttpContent", "System.Net.Http")] |
There was a problem hiding this comment.
All of these are now identified by the linker because of the helper type annotation?
There was a problem hiding this comment.
Only
[DynamicDependency("#ctor", "System.Net.Http.HttpRequestMessage", "System.Net.Http")]
is now identified by the linker because of the helper type annotation below.
The rest are unnecessary because the linker is able to directly recognize the types and ctors/properties/methods being used in this method. When it can't recognize the Reflection usage, it warns (like it was previously with httpRequestMessageType).
| { | ||
| HttpRequestMessageType = httpRequestMessageType; | ||
| } | ||
| } |
There was a problem hiding this comment.
It's unfortunate that the best way to prevent linker warnings is to add more overhead at runtime (and actually increase binary size with this additional class).
Did you consider just suppressing the warning? It was safe previously and just issuing warnings because the linker couldn't figure that out, right?
There was a problem hiding this comment.
Suppressing it is kind of hard because one of the warnings comes from inside the lambda method. So placing the suppression attribute would need to suppress more than necessary.
And in general, the recommendation from @vitek-karas and @MichalStrehovsky has been to favor [DAM] attributes over DynamicDependency and Suppress. The reasoning being is that the linker can verify you've done it correctly. Suppressing the warning could lead to bugs in the future if the code is changed and new warnings get suppressed.
Another approach I thought of, but didn't try was to store HttpRequestMessageType on a static field with the [DAM] annotation. I shied away from it because of nullability hoops I'd have to jump through. That would only add an extra reference/pointer statically, instead of a whole new type and object instance here. Thoughts on that approach?
There was a problem hiding this comment.
Another approach would be to do the closure manually - that would let you add the necessary annotation to the field. The compiler here generates a type and so on for the closure for the returned lambda. That could be done manually... It would avoid the second new type (only the closure type would remain). But it would make the code less readable/maintainable.
You could also simplify the helper type - make it a struct and make the type a field - to reduce the metadata size impact.
I can't see how linker could figure this out without cross-method analysis unfortunately.
There was a problem hiding this comment.
I would grab a ConstructorInfo for the default constructor in the main method body and just Invoke it in the lambda. No DynamicallyAccessed annotations necessary and no extra closures. Illink should be able to figure everything out.
ConstructorInfo is a good replacement for Activator unless the type in question is a valuetype with no explicit default ctor (in which case we can't get a ConstructorInfo, but the type can still be CreateInstance'd).
I wish C# had a way to annotate the closure though :(.
There was a problem hiding this comment.
I changed it to use ConstructorInfo instead.
This has the drawback of not being able to take advantage of the perf improvements we've made recently in Activator.CreateInstance (#32520). But we are hitting an HTTP endpoint, so the perf here won't really be noticeable. Also, when we add Activator.CreateFactory (#36194), we can update this code to use the new API.
| { | ||
| HttpRequestMessageType = httpRequestMessageType; | ||
| } | ||
| } |
There was a problem hiding this comment.
Another approach would be to do the closure manually - that would let you add the necessary annotation to the field. The compiler here generates a type and so on for the closure for the returned lambda. That could be done manually... It would avoid the second new type (only the closure type would remain). But it would make the code less readable/maintainable.
You could also simplify the helper type - make it a struct and make the type a field - to reduce the metadata size impact.
I can't see how linker could figure this out without cross-method analysis unfortunately.
Use ConstructorInfo to create HttpRequestMessage instances.
|
Test failures are #47374. |
ghost
locked as resolved and limited conversation to collaborators
Contributes to #45623