-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 #86161. Related to #86438. Feature design in #81741.
- Loading branch information
1 parent
595cbd1
commit ac3979a
Showing
13 changed files
with
1,345 additions
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
141 changes: 141 additions & 0 deletions
141
src/tools/illink/src/linker/Linker.Steps/UnsafeAccessorMarker.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Runtime.CompilerServices; | ||
| using Mono.Cecil; | ||
|
|
||
| namespace Mono.Linker.Steps | ||
| { | ||
| // This class only handles static methods (all the unsafe accessors should be static) | ||
| // so there's no problem with forgetting the implicit "this". | ||
| #pragma warning disable RS0030 // MethodReference.Parameters is banned | ||
|
|
||
| readonly struct UnsafeAccessorMarker (LinkContext context, MarkStep markStep) | ||
| { | ||
| readonly LinkContext _context = context; | ||
| readonly MarkStep _markStep = markStep; | ||
|
|
||
| // We don't perform method overload resolution based on list of parameters (or return type) for now | ||
| // Mono.Cecil's method resolution is problematic and has bugs. It's also not extensible | ||
| // and we would need that to correctly implement the desired behavior around custom modifiers. So for now we decided to not | ||
| // duplicate the logic to tweak it and will just mark entire method groups. | ||
|
|
||
| public void ProcessUnsafeAccessorMethod (MethodDefinition method) | ||
| { | ||
| if (!method.IsStatic || !method.HasCustomAttributes) | ||
| return; | ||
|
|
||
| foreach (CustomAttribute customAttribute in method.CustomAttributes) { | ||
| if (customAttribute.Constructor.DeclaringType.FullName == "System.Runtime.CompilerServices.UnsafeAccessorAttribute") { | ||
| if (customAttribute.HasConstructorArguments && customAttribute.ConstructorArguments[0].Value is int kindValue) { | ||
| UnsafeAccessorKind kind = (UnsafeAccessorKind) kindValue; | ||
| string? name = null; | ||
| if (customAttribute.HasProperties) { | ||
| foreach (CustomAttributeNamedArgument prop in customAttribute.Properties) { | ||
| if (prop.Name == "Name") { | ||
| name = prop.Argument.Value as string; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| switch (kind) { | ||
| case UnsafeAccessorKind.Constructor: | ||
| ProcessConstructorAccessor (method, name); | ||
| break; | ||
| case UnsafeAccessorKind.StaticMethod: | ||
| ProcessMethodAccessor (method, name, isStatic: true); | ||
| break; | ||
| case UnsafeAccessorKind.Method: | ||
| ProcessMethodAccessor (method, name, isStatic: false); | ||
| break; | ||
| case UnsafeAccessorKind.StaticField: | ||
| ProcessFieldAccessor (method, name, isStatic: true); | ||
| break; | ||
| case UnsafeAccessorKind.Field: | ||
| ProcessFieldAccessor (method, name, isStatic: false); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
| // Intentionally only process the first such attribute | ||
| // if there's more than one runtime will fail on it anyway. | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void ProcessConstructorAccessor (MethodDefinition method, string? name) | ||
| { | ||
| // A return type is required for a constructor, otherwise | ||
| // we don't know the type to construct. | ||
| // Types should not be parameterized (that is, by-ref). | ||
| // The name is defined by the runtime and should be empty. | ||
| if (method.ReturnsVoid () || method.ReturnType.IsByRefOrPointer () || !string.IsNullOrEmpty (name)) | ||
| return; | ||
|
|
||
| if (_context.TryResolve (method.ReturnType) is not TypeDefinition targetType) | ||
| return; | ||
|
|
||
| foreach (MethodDefinition targetMethod in targetType.Methods) { | ||
| if (!targetMethod.IsConstructor || targetMethod.IsStatic) | ||
| continue; | ||
|
|
||
| _markStep.MarkMethodVisibleToReflection (targetMethod, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method)); | ||
| } | ||
| } | ||
|
|
||
| void ProcessMethodAccessor (MethodDefinition method, string? name, bool isStatic) | ||
| { | ||
| // Method access requires a target type. | ||
| if (method.Parameters.Count == 0) | ||
| return; | ||
|
|
||
| if (string.IsNullOrEmpty (name)) | ||
| name = method.Name; | ||
|
|
||
| TypeReference targetTypeReference = method.Parameters[0].ParameterType; | ||
| if (_context.TryResolve (targetTypeReference) is not TypeDefinition targetType) | ||
| return; | ||
|
|
||
| if (!isStatic && targetType.IsValueType && !targetTypeReference.IsByReference) | ||
| return; | ||
|
|
||
| foreach (MethodDefinition targetMethod in targetType.Methods) { | ||
| if (targetMethod.Name != name || targetMethod.IsStatic != isStatic) | ||
| continue; | ||
|
|
||
| _markStep.MarkMethodVisibleToReflection (targetMethod, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method)); | ||
| } | ||
| } | ||
|
|
||
| void ProcessFieldAccessor (MethodDefinition method, string? name, bool isStatic) | ||
| { | ||
| // Field access requires exactly one parameter | ||
| if (method.Parameters.Count != 1) | ||
| return; | ||
|
|
||
| if (string.IsNullOrEmpty (name)) | ||
| name = method.Name; | ||
|
|
||
| if (!method.ReturnType.IsByReference) | ||
| return; | ||
|
|
||
| TypeReference targetTypeReference = method.Parameters[0].ParameterType; | ||
| if (_context.TryResolve (targetTypeReference) is not TypeDefinition targetType) | ||
| return; | ||
|
|
||
| if (!isStatic && targetType.IsValueType && !targetTypeReference.IsByReference) | ||
| return; | ||
|
|
||
| foreach (FieldDefinition targetField in targetType.Fields) { | ||
| if (targetField.Name != name || targetField.IsStatic != isStatic) | ||
| continue; | ||
|
|
||
| _markStep.MarkFieldVisibleToReflection (targetField, new DependencyInfo (DependencyKind.UnsafeAccessorTarget, method), new MessageOrigin (method)); | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.