[coop][interp] Fix GC transitions for thunk invoke wrappers by lambdageek · Pull Request #81773 · dotnet/runtime

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek · 2023-02-07T18:46:28Z

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced

Change the thunk invoke wrapper to also use attach_coop/detach_coop. This makes the thunks a bit more flexible (they can now be called from unattached threads), at the cost of a TLS lookup. Also fixes interpreter support since they use the existing attach/detach handling.

As an implementation detail, I added a GC Unsafe Transition Builder to the marshaling code.

Fixes a failure seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see mono_threads_enter_gc_safe_region_unbalanced / mono_threads_exit_gc_safe_region_unbalanced which are used by managed-to-native wrappers. In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod. However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses mono_threads_enter_gc_unsafe_region_unbalanced / mono_threads_exit_gc_unsafe_region_unbalanced Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

ghost · 2023-02-07T18:46:51Z

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

Issue Details

The code that recognizes GC transition icalls in the interpreter was only assuming that it will see
mono_threads_enter_gc_safe_region_unbalanced /
mono_threads_exit_gc_safe_region_unbalanced

which are used by managed-to-native wrappers.

In most cases for native-to-managed wrappers the marshaller emits mono_threads_attach_coop / mono_threads_detach_coop and those are handled elsewhere by setting the needs_thread_attach flag on the InterpMethod.

However when the mono_marshal_get_thunk_invoke_wrapper API is used, we emit a thunk invoke wrapper which uses
mono_threads_enter_gc_unsafe_region_unbalanced /
mono_threads_exit_gc_unsafe_region_unbalanced

Recognize those two icalls and set the needs_thread_attach flag. (This is slightly more work than necessary - mono_threads_attach_coop checks if the thread was previously attached to the runtime and attaches it if it wasn't. The thunk invoke wrappers seem to assume the API caller attaches the thread - so we pay for an extra check. But on the other hand we don't need to change the execution-time behavior of the interpreter by reusing existing mechanisms)

Fixes a failures seen in #81380 in the MonoAPI/MonoMono/Thunks test (exposed by calling a cctor later than previously)

Author:	lambdageek
Assignees:	lambdageek
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2023-02-07T21:15:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-07T21:16:05Z

Azure Pipelines successfully started running 1 pipeline(s).

BrzVlad · 2023-02-08T12:56:13Z

I don't really understand how exactly your other PR induces this failure. Nevertheless, silently doing an attach behind the scenes seems like a hack to me. I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost. Otherwise it sounds like the test has a bug and it relies on some thread being attached. Or maybe the cctor invocation order is not really correct.

lambdageek · 2023-02-08T14:22:02Z

silently doing an attach behind the scenes seems like a hack to me.

My point is, there is no attach happening. Any user code that calls the thunk returned by mono_marshal_get_thunk_invoke_wrapper must already have done an attach. So mono_thread_attach_coop is just to do the GC Safe -> GC Unsafe transition.

I think we can either choose to perform explicit attach during thunk invoke wrappers, since it sounds like it doesn't really have a perf cost.

Yea I guess I could just change emit_thunk_invoke_wrapper to emit attach/detach pairs like all the other thunks. It would be a change of behavior for existing code, but correct code shouldn't notice. and incorrect code will be upgraded to be correct.

lambdageek · 2023-02-08T14:26:16Z

I don't really understand how exactly your other PR induces this failure

It's the bit about not calling the managed ALC callback for the default ALC. Early during startup we used to end up in there and run the cctor for the default ALC. That would trigger a bunch of other early initialization. With my PR, we don't call that managed callback for the default ALC anymore because it is just the default implementation which always returns NULL. As a result a lot fewer cctors run at startup.

In the Thunks.cs test, the cctor for NotImplementedException is one of those cctors. After the PR, that cctor now runs when we call back into managed after the thunk_invoke_wrapper. And when cctors run they need to take a lock in mono - and attempt to do a GC Unsafe -> GC Safe transition. Under the interp, the thunk invoke wrapper was missing the GC Safe -> GC Unsafe transition on entry, so the cctor locking was doing GC Safe -> GC Safe, which is illegal. It worked on AOT and JIT because those had the correct transition in the wrapper.

This reverts commit 583ad5a.

Only assert in debug builds

Make a "GC Unsafe Transition Builder" - that always calls mono_threads_attach_coop / mono_threads_detach_coop. Use it in the native to managed wrappers: emit_thunk_invoke_wrapper and emit_managed_wrapper This is a change in behavior for emit_thunk_invoke_wrapper - previously it directly called mono_threads_enter_gc_unsafe_region_unbalanced. That means that compared to the previous behavior, the thunk invoke wrappers are now a bit more lax: they will be able to be called on threads that aren't attached to the runtime and they will attach automatically. On the other hand existing code will continue to work, with the extra cost of a check of a thread local var. Using mono_thread_attach_coop also makes invoke wrappers work correctly in the interpreter - it special cases mono_thread_attach_coop but not enter_gc_unsafe.

lambdageek · 2023-02-10T17:26:13Z

@BrzVlad I updated the PR to change the thunk invoke wrappers to use attach_coop/detach_coop and fall into the existing attach/detach handling in the interpreter

lambdageek · 2023-02-10T19:47:22Z

update oops, wrong PR

lambdageek requested review from BrzVlad, kotlarmilos and vargaz as code owners February 7, 2023 18:46

ghost added the area-Codegen-Interpreter-mono label Feb 7, 2023

ghost assigned lambdageek Feb 7, 2023

lambdageek mentioned this pull request Feb 7, 2023
[mono] Implement synch block fast paths in managed #81380
Merged

lambdageek added 3 commits February 10, 2023 10:16

Revert "[coop][interp] Fix GC transitions for thunk invoke wrappers"
0510e90
This reverts commit 583ad5a.

Assert that CEE_MONO_GET_SP is followed by gc safe enter/exit icalls
495bdab
Only assert in debug builds

lambdageek requested a review from thaystg as a code owner February 10, 2023 17:23

lambdageek requested a review from naricc February 10, 2023 18:16

lambdageek added area-Interop-mono and removed area-Codegen-Interpreter-mono labels Feb 10, 2023

BrzVlad approved these changes Feb 13, 2023
View reviewed changes

lambdageek merged commit 9f8c009 into dotnet:mainFeb 13, 2023

ghost locked as resolved and limited conversation to collaborators Mar 15, 2023