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

[Mono] Implement support for UnsafeAccessorAttribute #86040

Closed
7 tasks done
lambdageek opened this issue May 10, 2023 · 10 comments
Closed
7 tasks done

[Mono] Implement support for UnsafeAccessorAttribute #86040

lambdageek opened this issue May 10, 2023 · 10 comments
Assignees
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented May 10, 2023

Implement support for "zero overhead member access with suppressed visibility checks" #81741 for the mono JIT/AOT and interp

There might be some mild complication here for working with generics (the static extern method will be generic in that case) - our ilgen method builder might not support generics (in which case it will have to be extended)

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 10, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 10, 2023
@lambdageek lambdageek added area-Codegen-meta-mono and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 10, 2023
@lambdageek lambdageek added this to the 8.0.0 milestone May 10, 2023
@lambdageek
Copy link
Member Author

/cc @SamMonoRT

@lambdageek
Copy link
Member Author

lambdageek commented May 10, 2023

It doesn't look like we will need anything special to skip accessibility checks for fields

For something like

class MyClass {
  private MyFieldType MyField;
  ...
}

[UnasafeAccessor(UnsafeAccessorKind.Field, Name="MyField")]
static extern ref MyFieldType GetMyField(MyClass c);

the ilgen could use mono_mb_emit_ldarg and mono_mb_emit_ldflda which would emit

  ldarg.0
  mono_custom_prefix . mono_objaddr
  add <field_offset>

for method calls, mono_mb_emit_managed_call just uses a normal IL call instruction (nit: we actually want callvirt) so we might need some prefix opcode to skip accessibility checks. (or we understand that this ilgen'd method should skip accessibility checks entirely - I think we already have that for wrappers)

@lambdageek
Copy link
Member Author

For generic field access we can't use mono_objaddr + add <constant>, so we'll have to use a real ldflda instruction. Which probably means we should just have a tag on the method to bypass access checks and then do things with normal IL opcodes.

@AaronRobinsonMSFT
Copy link
Member

@lambdageek I've added #86161 for CoreCLR.

@fanyang-mono fanyang-mono self-assigned this May 12, 2023
@lambdageek
Copy link
Member Author

There's also a question of when do we create the IL for this "extern" method which is sort of interesting. I think for JIT/AOT we can actually do it during the call to mono_compile_method_checked. Not sure when to do it in the interpreter.

I guess the question is whether the same logic that we use for dllimports and icalls will work for plain "extern" methods. (I'm pretty sure that's not the case - we look for the internalcall or pinvokeimpl flags, not just methods with no code)

@lambdageek
Copy link
Member Author

lambdageek commented May 30, 2023

I guess the thing to pattern this on is where we call mono_marshal_get_delegate_begin_invoke: in mono_interp_transform_method and compile_special

and in general we probably should treat these as special kind of wrappers, kind of like MONO_WRAPPER_DELEGATE_BEGIN_INVOKE.

So we should add new subtypes of MONO_WRAPPER_OTHER for unsafe accessors and use the wrapper emit logic (incl caching) to generate the bodies. it just happens that the wrapper does all the real work and the underlying method has no code.

@lambdageek
Copy link
Member Author

From #81741 (comment):

The plan has been to avoid special casing these extern methods with this attribute in Roslyn in any way. It means that these extern methods would be MethodImplAttributes.IL just like any other extern method.

In CoreCLR, we plan to check for zero RVA first, and then for the attribute if the method has zero RVA. If a method with non-zero RVA is marked with this attribute, it is going to be no-op. We do not plan to do sanity checks for invalid use of this attribute on methods with non-zero RVA.

@AaronRobinsonMSFT
Copy link
Member

@lambdageek See the updates to jitinterface.cpp. The final else branch in getMethodInfoHelper() is when we generate this information (that is, at JIT time). It calls MethodDesc::GenerateUnsafeAccessor() in prestub.cpp.

@fanyang-mono
Copy link
Member

All the work planned for .NET8 has been completed. Moving this tracking issue to 9.0.0

@jkotas
Copy link
Member

jkotas commented Jul 25, 2023

This can be closed. The remaining work is tracked by #89439

@jkotas jkotas closed this as completed Jul 25, 2023
@fanyang-mono fanyang-mono modified the milestones: 9.0.0, 8.0.0 Jul 25, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants