Skip to content

Conversation

@thaystg
Copy link
Member

@thaystg thaystg commented Jun 9, 2020

  • Implementing QCalls on mono
  • Use QCalls on ICU Shim implementation

Fixes #36449

@thaystg thaystg requested a review from lambdageek June 10, 2020 20:55
Comment on lines 21 to 26
enum {
func_flag_end_of_array = 0x01,
func_flag_has_signature = 0x02,
func_flag_unreferenced = 0x04, // Suppress unused fcall check
func_flag_qcall = 0x08, // QCall - mscorlib.dll to mscorwks.dll transition implemented as PInvoke
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really follow Mono's coding conventions (no mono prefix, etc). Also we only need 0x01 and 0x08, right? could we leave out the other two: comment them out in case we need to find them later.

These flags are an implementation detail of the qcall table, right? We're not obligated to follow the coreclr implementation verbatim. so we could make MonoQCallFunc:flags anything we want.

Copy link
Member

@lambdageek lambdageek Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see https://github.com/dotnet/runtime/blob/master/src/coreclr/src/libraries-native/entrypoints.c is shared with CoreCLR. So we are tied to their naming for the global array of PAL qcalls, and to their layout of the array data.

Let's have a comment about it

@lambdageek
Copy link
Member

@lateralusX could you take a look at the eventpipe bits

func_flag_qcall = 0x08, // QCall - mscorlib.dll to mscorwks.dll transition implemented as PInvoke
};

const MonoQCallDef c_qcalls[] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a comment here about the sorting requirements of FCClassEment and QCFuncElement definitions

@lambdageek lambdageek changed the title Add QCalls to the Runtime [mono] Add QCalls to the Runtime Jun 11, 2020
@lambdageek lambdageek added the runtime-mono specific to the Mono runtime label Jun 11, 2020
const gunichar2 *provider_name,
void* callback_func)
{
BEGIN_QCALL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the eventpipe former icalls we also need

Suggested change
BEGIN_QCALL;
BEGIN_QCALL;
MONO_ENTER_GC_UNSAFE;

(and the corresponding exit near the END_QCALL).
The former icalls work assuming that they are in GC Unsafe mode, while QCalls are just P/Invokes and they will be in GC Safe mode on entry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that this will be true of qcalls in general. (for example the ICU shim ones are fine in GC Safe). So I'm not sure if it makes sense to just make BEGIN_QCALL do the state transition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of QCalls is that they are just like regular PInvokes and the code inside them is just like arbitrary 3rd party code. Why would we want have MONO_ENTER_GC_UNSAFE there? Having different contracts for the code on Mono vs. CoreCLR will make sharing the code hard.

The BEGIN/END_QCALL macro is there to convert C++ exceptions into managed exceptions. Since you are not using C++ exceptions for error handling in Mono, I think you can omit this macro from all code converted to C. I assume that the code converted to C is not going to be using exception handling for error handling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, no exception handling is used, just pure C, but current integration into runtime from within EventPipe library uses some functions (like Mono's w32 API's) that assumes GC unsafe for some situations, so that needs to be changed in ep-rt-mono.h before we can switch from icalls to qcalls. Maybe we should split the EventPipe library switch to qcalls into separate PR that also make sure we do the needed changes into EventPipe library necessary to make the switch to qcalls and align to GC safe mode. Since the port of the complete EventPipe library is not 100% complete yet (at least #37756), we could still progress on the parts that are not EventPipe specific and then finalize the use of qcalls for EventPipe library at a later point in time.

Copy link
Member

@lateralusX lateralusX Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that the GC state is transparent for the majority of the EventPipe C code, this is only part of the runtime specific ep-rt.h inlinable shim, so majority of EventPipe code will run under either modes and since it hopefully will be shared between runtime going forward, using same GC mode (GC safe) and qcalls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a test here, the way that is implemented in the PR, when we call a QCall function, the return of mono_thread_info_current_state is STATE_BLOCKING, as I understood it's GC_SAFE. If we want to change to GC_UNSAFE we should use SuppressGCTransition in the definition in the c# file, maybe we don't need this in BEGIN/END_QCALL macro.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add attribute to the C# file we will need to condition it differently between runtimes, since CoreCLR assumes GC safe for EventPipe library. I think we keep icalls in EventPipe library for now and then make a switch using QCalls switching Mono runtime integration to only use GC safe methods in ep-rt-mono.h in one PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's two separate issues:

  1. Does calling a QCall from managed code do a GC Unsafe -> GC Safe transition? Yes: QCalls are normal PInvokes. This behavior also makes sense for a lot of QCalls that are like the ICU shims or like a lot of calls in System.Native - they are not really interacting with the runtime in any meaningful way.
  2. Once in a QCall when do you switch to GC Unsafe? In Mono the answer is "as soon as possible when you need to call any runtime _internal function". Most of the runtime code assumes you're in GC Unsafe. All of our locks assume you're in GC Unsafe and try to switch to GC Safe just around the blocking calls, for example.

When we initially added hybrid suspension to Mono, we thought about having the runtime stay mostly in GC Safe mode except when interacting with the managed heap, and it was basically incompatible with our existing programming model (raw pointers to MonoObjects abound). So in Mono you have to switch to GC Unsafe pretty early on if you're doing anything that interacts with the runtime internals.

I'm not sure how EventPipe will play out - it might make sense to keep the code mostly GC Safe and only switch to unsafe in the runtime-specific interface code.

@lateralusX
Copy link
Member

lateralusX commented Jun 12, 2020

@thaystg Maybe we should break out EventPipe specific work done in this PR into separate PR that can be completed and merged once the port of the library is complete? That way we can do the adjustments of the EventPipe library to run under GC safe mode as part of that other PR and no need to implement a short lived temporary solution in this PR.

@thaystg
Copy link
Member Author

thaystg commented Jun 12, 2020

@thaystg Maybe we should break out EventPipe specific work done in this PR into separate PR that can be completed and merged once the port of the library is complete? That way we can do the adjustments of the EventPipe library to run under GC safe mode as part of that other PR and no need to implement a short lived temporary solution in this PR.

Okay, I will remove EventPipe changes...

@thaystg thaystg marked this pull request as ready for review June 15, 2020 14:51
@thaystg thaystg requested review from CoffeeFlux, jkotas and vargaz June 16, 2020 13:24
Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a full pass for other style nits. Otherwise, LGTM—thanks!

thaystg and others added 16 commits June 17, 2020 09:11
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc: @jkotas

@thaystg thaystg merged commit fae477f into dotnet:master Jun 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Interop-mono runtime-mono specific to the Mono runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Mono] Add QCalls to the runtime

8 participants