Implement support for `UnsafeAccessor` in the trimmer (#88268)
authorVitek Karas <10670590+vitek-karas@users.noreply.github.com>
Tue, 18 Jul 2023 07:33:40 +0000 (00:33 -0700)
committerGitHub <noreply@github.com>
Tue, 18 Jul 2023 07:33:40 +0000 (00:33 -0700)
commitac3979aca410dc33396c95b1fe4b85424900f603
treeff68046a96095148f5a554caee285c9c418b7baf
parent595cbd1c8d40614684540585424ca7027125ff51
Implement support for `UnsafeAccessor` in the trimmer (#88268)

The core of the change is that `UnsafeAccessor` creates a code dependency from the accessor method to the target specified by the attribute. The trimmer needs to follow this dependency and preserve the target. Additionally, because the trimmer operates at the IL level, it needs to make sure that the target will keep its name and some other properties intact (so that the runtime implementation of the `UnsafeAccessor` can still work).

Implementation choices:
* The trimmer will mark the target as "accessed via reflection", this is a simple way to make sure that name and other properties about the target are preserved. This could be optimized in the future, but the savings are probably not that interesting.
* The implementation ran into a problem when trying to precisely match the signature overload resolution. Due to Cecil issues and the fact that Cecil's resolution algorithm is not extensible, it was not possible to match the runtime's behavior without adding lot more complexity (currently it seems we would have to reimplement method resolution in the trimmer). So, to simplify the implementation, trimmer will mark all methods of a given name. This means it will mark more than necessary. This is fixable by adding more complexity to the code base if we think there's a good reason for it.
* Due to the above choices, there are some behavioral differences:
  * Trimmer will warn if the target has data flow annotations, always. There's no way to "fix" this in the code without a suppression.
  * Trimmer will produce different warning codes even if there is a true data flow mismatch - this is because it treats the access as "reflection access" which produces different warning codes from direct access.
  * These differences are fixable, but it was not deemed necessary right now.
* We decided that analyzer will not react to the attribute at all, and thus will not produce any diagnostics around it.

The guiding reason to keep the implementation simple is that we don't expect the unsafe accessor to be used by developers directly, instead we assume that vast majority of its usages will be from source generators. So, developer UX is not as important.

Test changes:
* Adds directed tests for the marking behavior
* Adds tests to verify that `Requires*` attributes behave correctly
* Adds tests to verify that data flow annotations behave as expected (described above)
* The tests are effectively a second validation of the NativeAOT implementation as they cover NativeAOT as well.

Fixes in CoreCLR/NativeAOT:
This change fixes one bug in the CoreCLR/NativeAOT implementation, unsafe accessor on a instance method of a value type must use "by-ref" parameter for the `this` parameter. Without the "by-ref" the accessor is considered invalid and will throw.
This change also adds some tests to the CoreCLR/NativeAOT test suite.

Part of https://github.com/dotnet/runtime/issues/86161.
Related to https://github.com/dotnet/runtime/issues/86438.
Feature design in https://github.com/dotnet/runtime/issues/81741.
13 files changed:
eng/pipelines/runtime-linker-tests.yml
src/coreclr/tools/Common/TypeSystem/IL/UnsafeAccessors.cs
src/coreclr/tools/aot/Mono.Linker.Tests/TestCases/TestSuites.cs
src/coreclr/vm/prestub.cpp
src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs
src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
src/tools/illink/src/linker/Linker.Steps/UnsafeAccessorMarker.cs [new file with mode: 0644]
src/tools/illink/src/linker/Linker/DependencyInfo.cs
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/ReflectionTests.g.cs
src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaUnsafeAccessor.cs [new file with mode: 0644]
src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/UnsafeAccessor.cs [new file with mode: 0644]
src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresAccessedThrough.cs