Uh oh!
There was an error while loading. Please reload this page.
Share return value handling in trim analysis - #101398
Conversation
| [ExpectedWarning ("IL2075", "GetMethod")] | ||
| public static void Test () | ||
| { | ||
| new Derived ().GetType ().GetMethod ("Method"); |
There was a problem hiding this comment.
The analyzer handling of object.GetType was not setting a return value, so moving to the shared logic fixes this discrepancy.
Uh oh!
There was an error while loading. Please reload this page.
And add missing cases to the switch statement for the analyzer.
sbomer
commented
Apr 24, 2024
I missed a few cases for the intrinsics that don't have special analyzer handling. I added those cases, and also made a fix to avoid crashing Release builds of the analyzer in case we add more cases without analyzer handling in the future. |
The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
sbomer
commented
Apr 24, 2024
@MichalStrehovsky I'd appreciate your eyes on the latest commit b898b27. |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
LGTM otherwise, thanks!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| IntrinsicId intrinsicId, | ||
| out MultiValue? methodReturnValue) | ||
| { | ||
| MultiValue? maybeMethodReturnValue = methodReturnValue = null; |
There was a problem hiding this comment.
What's the difference between maybeMethodReturnValue and methodReturnValue (do we need both?)
There was a problem hiding this comment.
Some code paths assign to the out param methodReturnValue and return immediately, but the paths that use AddReturnValue needed a separate local because local functions can't assign to out parameters of the containing function.
- Return instead of out param in HandleCall - FIx formatting
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed. * Fix Array_CreateInstance case The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed. * Fix Array_CreateInstance case The PInvoke logic and the CheckAndReportRequires logic should not be needed for this intrinsic, and the old shared HandleCallAction would go to the default case that sets the return value to Top and returns false. Now this instead returns true and lets the shared return value logic kick in.
This shares more of the HandleCall logic across ILLink/ILC/ILLink.RoslynAnalyzer. The return value handling has been moved from each tool's implementation (ReflectionMethodBodyScanner/TrimAnalysisVisitor) into the shared HandleCallAction, and the extra handling in MethodBodyScanner has been removed.
Addresses the suggestions from #101212 (comment) and #101212 (comment).