Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ILStrip task incorrectly adds a body to extern methods #102045

Closed
lambdageek opened this issue May 9, 2024 · 3 comments · Fixed by dotnet/runtime-assets#475
Closed

ILStrip task incorrectly adds a body to extern methods #102045

lambdageek opened this issue May 9, 2024 · 3 comments · Fixed by dotnet/runtime-assets#475

Comments

@lambdageek
Copy link
Member

lambdageek commented May 9, 2024

If you run the ILStrip task on an assembly that contains an extern method that has no body or header, ILStrip adds one back.

public class AccessHelper<W>
{
        [UnsafeAccessor(UnsafeAccessorKind.Constructor)]
        public static extern MyList<W> CreateSized(int n);
}

Original IL:

.class public auto ansi beforefieldinit AccessHelper`1<W>
       extends [System.Runtime]System.Object
{
  .method public hidebysig static class MyList`1<!W>
          CreateSized(int32 n) cil managed
  {
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.UnsafeAccessorAttribute::.ctor([System.Runtime]System.Runtime.CompilerServices.UnsafeAccessorKind) = ( 01 00 00 00 00 00 00 00 )
  } // end of method AccessHelper`1::CreateSized

...
}

After ILStrip:

class public auto ansi beforefieldinit AccessHelper`1<W>
       extends [System.Private.CoreLib]System.Object
{
  .method public hidebysig static class MyList`1<!W>
          CreateSized(int32 A_0) cil managed noinlining
  {
    .custom instance void [System.Private.CoreLib]System.Runtime.CompilerServices.UnsafeAccessorAttribute::.ctor([System.Private.CoreLib]System.Runtime.CompilerServices.UnsafeAccessorKind) = ( 01 00 00 00 00 00 00 00 )
    // Code size       1 (0x1)
    .maxstack  8
    IL_0000:  ret
  } // end of method AccessHelper`1::CreateSized

} // end of class AccessHelper`1

This affects UnsafeAccessor support on Apple mobile platforms (both the runtime samples and xamarin-macios use the ILStrip task). related to #89439, #99830

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 9, 2024
Copy link
Contributor

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

@lambdageek lambdageek added this to the 9.0.0 milestone May 9, 2024
@lambdageek lambdageek added area-Infrastructure-mono and removed area-System.Runtime.CompilerServices untriaged New issue has not been triaged by the area owner labels May 9, 2024
Copy link
Contributor

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

@lambdageek
Copy link
Member Author

The reason this affects Mono's [UnsafeAccessor] support is because it is cheaper for us to check "does this method have a body" than to check "does this method have a particular custom attribute". Since the majority of methods have bodies and are not unsafe accessors, we use the has-a-body check to quickly bypass the slow path of working with the custom attribute

steveisok added a commit to dotnet/runtime-assets that referenced this issue Aug 7, 2024
The prior behavior assumed a MethodDefinition's RVA value of 0 meant an effectively empty body with a single RET instruction. This change removes the RET insertion and leaves the method completely empty. This is important for mono's UnsafeAccessor detection around methods specified as extern as it uses a quicker method of an empty vs non-empty body to bypass loading / checking custom attributes.

Fixes dotnet/runtime#102045
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant