Skip to content

[MONO][MARSHAL] Initialize ilgen with a flag#77448

Merged
naricc merged 7 commits into
dotnet:mainfrom
naricc:naricc/marshal-flag
Oct 31, 2022
Merged

[MONO][MARSHAL] Initialize ilgen with a flag#77448
naricc merged 7 commits into
dotnet:mainfrom
naricc:naricc/marshal-flag

Conversation

@naricc

@naricc naricc commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

Currently, marshal ilgen callbacks are initialized lazily or by having embedders call mono_marshal_ilgen_init, and the code for doing this has a race condition. This change modifies mono_marshal_ilgen_init so that it sets a flag, and if the flag is set either ilgen callbacks are installed later during startup (intead of lazily). If the flag is not set, and ENABLE_ILGEN is false, the noilgen callbacks are installed instead.

Installation of callbacks occurs only once before any user code is executing, so there is no longer a race condition. See discussion here: #77383 (comment)

Fixes: #74603
Fixes: #77090

@naricc
naricc force-pushed the naricc/marshal-flag branch from b1dae36 to 09902c0 Compare October 25, 2022 20:13
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek lambdageek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

Comment thread src/mono/mono/mini/mini-runtime.c
Comment thread src/mono/mono/metadata/marshal-lightweight.h Outdated
Comment thread src/mono/mono/mini/mini-runtime.c Outdated
@dotnet dotnet deleted a comment from azure-pipelines Bot Oct 26, 2022
@dotnet dotnet deleted a comment from azure-pipelines Bot Oct 26, 2022
@naricc

naricc commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@naricc

naricc commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@naricc

naricc commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@naricc

naricc commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@naricc
naricc requested a review from lewing as a code owner October 28, 2022 03:31
@naricc
naricc requested a review from pavelsavara as a code owner October 28, 2022 03:31
@naricc
naricc requested a review from marek-safar as a code owner October 28, 2022 14:45
@naricc

naricc commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@naricc

naricc commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@naricc

naricc commented Oct 29, 2022

Copy link
Copy Markdown
Contributor Author

The remaining test failure appears to be this issue: #77630

However, I am reluctant to merge this until that is resolved. That issue prevents the whole test suite from running, since it stops the product build step, and errors in this change definitely have the possibility of breaking wasm.

@naricc

naricc commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

Well, it passed several other wasm lanes and the marshaling stuff doesn't vary on them. So I think this is good to merge.

@naricc
naricc merged commit 22ecd7e into dotnet:main Oct 31, 2022
@SamMonoRT

Copy link
Copy Markdown
Member

@naricc @lambdageek - we should give this fix a few days on main and then consider backporting to servicing release

@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2022
@jandupej

Copy link
Copy Markdown
Contributor

/backport to release/7.0

@github-actions github-actions Bot unlocked this conversation Mar 23, 2023
@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4500733710

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Mono] System.Text.RegularExpressions.Tests source generator tests crash in DeflateInit2_ Race condition on marshal-ilgen init

4 participants