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.

Sign up for GitHub

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

Jump to bottom

[mono] Make some icalls pass/return object references using ObjectHandleOnStack/QCallTypeHandle #62141

Merged
merged 17 commits into from
Jan 6, 2022
Merged

[mono] Make some icalls pass/return object references using ObjectHandleOnStack/QCallTypeHandle #62141

merged 17 commits into from
Jan 6, 2022

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Nov 29, 2021

…tack argument.

@vargaz vargaz marked this pull request as draft November 29, 2021 15:05
@ghost
Copy link

ghost commented Nov 29, 2021

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

Issue Details

…tack argument.

Author: vargaz
Assignees: -
Labels:

area-VM-meta-mono
Milestone: -

This was referenced Nov 29, 2021
Closed
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Dec 1, 2021

The wasm failures are relevant.

@vargaz vargaz force-pushed the qcalls branch 2 times, most recently from 548a3a2 to 87a6f09 Compare December 6, 2021 17:17
@vargaz
Copy link
Contributor Author

vargaz commented Dec 6, 2021

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Dec 6, 2021

The wasm failures are probably becaue ObjectHandleOnStack is a structure and its passed incorrectly as an argument to icalls on wasm.

@vargaz
Copy link
Contributor Author

vargaz commented Dec 20, 2021

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek
Copy link
Member

Pass ObjectHandleOnStack by ref, valuetypes cannot be passed to icalls on mono.

@vargaz I don't think this PR is worth it if we can't use the exact same managed declarations and callers that coreclr do.

We may need to teach our marshalling code that ObjectHandleOnStack is just an IntPtr

For example:

runtime/src/coreclr/System.Private.CoreLib/src/System/TypeNameParser.cs

Lines 49 to 50 in 99d82c2

[GeneratedDllImport(RuntimeHelpers.QCall, EntryPoint = "TypeName_GetModifiers")]
private static partial void _GetModifiers(SafeTypeNameParserHandle pTypeNameParser, ObjectHandleOnStack retArray);

runtime/src/coreclr/System.Private.CoreLib/src/System/TypeNameParser.cs

Line 326 in 99d82c2

_GetModifiers(m_NativeParser, ObjectHandleOnStack.Create(ref modifiers));

@vargaz
Copy link
Contributor Author

vargaz commented Dec 21, 2021

Some of the types passed are more complicated, QCallTypeReference is a struct with 2 fields for example.

@vargaz vargaz marked this pull request as ready for review December 22, 2021 05:59
@lambdageek
Copy link
Member

lambdageek commented Dec 22, 2021 •
edited
Loading

Some of the types passed are more complicated, QCallTypeReference is a struct with 2 fields for example.

I think I'm wrong: there's nothing special for marshaling. All these types are represented as blittable structs and are just passed using the normal ABI calling convention.

But also, QCalls are PInvokes, not icalls.
So (later, in other PRs) when we go to unify the managed side with CoreCLR, we will also need to tweak the native side: get the declarations out of icall-decl, into the QCall tables, and change the implementations to use MONO_ENTER_GC_UNSAFE/MONO_EXIT_GC_UNSAFE before they call into runtime internals.

@lambdageek
Copy link
Member

Is this the reference for the wasm32 ABI that Emscripten uses? https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md#function-calling-sequence

@vargaz
Copy link
Contributor Author

vargaz commented Dec 22, 2021

Yes, there was an issue passing scalar structs on wasm both in AOT and the interpreter, which is also fixed by this PR.

@vargaz
Copy link
Contributor Author

vargaz commented Jan 4, 2022

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vargaz and others added 11 commits January 4, 2022 15:02
@vargaz
[mono] Make some icalls return objects using an extra ObjectHandleOnS…
2d3c940 
…tack argument.
@vargaz
Fix the passing of scalar vtypes on wasm.
068fa6e
@vargaz
Convert RuntimeTypeHandle icalls to receive a QCallTypeHandle.
15e68e9 
Also do some other cleanups:
* Convert some icalls which don't receive/return objects any more and don't
  return an error to NOHANDLES.
* Implement IsGenericVariable in managed code.
* Add a separate GetMetadataToken icall to avoid the REUSE_WRAPPER stuff.
* Sync the implementation of IsTypeDefinition with coreclr.
@vargaz
Convert some RuntimeType icalls to use QCallTypeHandles/ObjectHandleO…
245b0eb 
…nStack.
@vargaz
Fix RuntimeType.GetPacking () to work with dynamic types.
0e94781
@vargaz
Convert internal_from_name to use QCallTypeHandles/ObjectHandleOnStack.
2c76dfd
@vargaz
Convert more icalls.
895fec5
@vargaz @lambdageek
Update src/libraries/System.Private.CoreLib/src/System/Runtime/Compil…
deec453 
…erServices/QCallHandles.cs

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
@vargaz
Remove unused argument from RuntimeTypeHandle:internal_from_name ().
6c98dff
@vargaz
Add support for FRAME_TYPE_IL_STATE to the metadata stack walker code.
972a3cf
@vargaz
Update coding style in QCallTypeHandle:.ctor ().
d1dd227
@vargaz
Copy link
Contributor Author

vargaz commented Jan 5, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Jan 5, 2022

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Jan 6, 2022

/azp run runtime-manual

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Jan 6, 2022

Failures are unrelated

@vargaz vargaz merged commit 01cc900 into dotnet:main Jan 6, 2022
@vargaz vargaz deleted the qcalls branch January 6, 2022 22:16
@ghost ghost locked as resolved and limited conversation to collaborators Feb 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@lambdageek lambdageek lambdageek approved these changes

@marek-safar marek-safar marek-safar left review comments

@BrzVlad BrzVlad Awaiting requested review from BrzVlad BrzVlad is a code owner

@imhameed imhameed Awaiting requested review from imhameed

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees
No one assigned
Labels
area-VM-meta-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vargaz @lambdageek @marek-safar @jkotas