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

[interp] inlining a wrapper with a MINT_LDSTR_TOKEN instruction aborts #88694

Closed
lambdageek opened this issue Jul 11, 2023 · 4 comments · Fixed by #88738
Closed

[interp] inlining a wrapper with a MINT_LDSTR_TOKEN instruction aborts #88694

lambdageek opened this issue Jul 11, 2023 · 4 comments · Fixed by #88738

Comments

@lambdageek
Copy link
Member

If the runtime creates a wrapper whose entire body is just mono_mb_emit_exception_full(mb, "System", "NotImplementedException", "some case in this wrapper is unimplemented") then the interpreter will emit an MINT_LDSTR_TOKEN instruction for the const char* msg argument.

If this wrapper is then inlined into its caller, we end up here:

MonoMethod *method = frame->imethod->method;
if (method->wrapper_type == MONO_WRAPPER_DYNAMIC_METHOD) {
s = (MonoString*)mono_method_get_wrapper_data (method, strtoken);
} else if (method->wrapper_type != MONO_WRAPPER_NONE) {
// FIXME push/pop LMF
s = mono_string_new_wrapper_internal ((const char*)mono_method_get_wrapper_data (method, strtoken));
} else {
g_assert_not_reached ();

Normally, if the wrapper is not inlined MonoMethod *method = frame->imethod->method will be the wrapper, and we end up in the method->wrapper_type != MONO_WRAPPER_NONE case.

But if the wrapper is inlined we end up in the else g_assert_not_reached() case and crash.

Found in an early iteration of #88626

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 11, 2023
@ghost
Copy link

ghost commented Jul 11, 2023

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

Issue Details

If the runtime creates a wrapper whose entire body is just mono_mb_emit_exception_full(mb, "System", "NotImplementedException", "some case in this wrapper is unimplemented") then the interpreter will emit an MINT_LDSTR_TOKEN instruction for the const char* msg argument.

If this wrapper is then inlined into its caller, we end up here:

MonoMethod *method = frame->imethod->method;
if (method->wrapper_type == MONO_WRAPPER_DYNAMIC_METHOD) {
s = (MonoString*)mono_method_get_wrapper_data (method, strtoken);
} else if (method->wrapper_type != MONO_WRAPPER_NONE) {
// FIXME push/pop LMF
s = mono_string_new_wrapper_internal ((const char*)mono_method_get_wrapper_data (method, strtoken));
} else {
g_assert_not_reached ();

Normally, if the wrapper is not inlined MonoMethod *method = frame->imethod->method will be the wrapper, and we end up in the method->wrapper_type != MONO_WRAPPER_NONE case.

But if the wrapper is inlined we end up in the else g_assert_not_reached() case and crash.

Found in an early iteration of #88626

Author: lambdageek
Assignees: -
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@lambdageek lambdageek changed the title [interp] inlining a wrapper with an LDSTR_TOKEN instruction aborts [interp] inlining a wrapper with a MINT_LDSTR_TOKEN instruction aborts Jul 11, 2023
@lambdageek
Copy link
Member Author

/cc @BrzVlad

@lambdageek
Copy link
Member Author

Actually it might not have anythign to do with inlining.

It has to do with the fact that imethod->method is always the wrapped method, not the wrapper

method = nm;
header = interp_method_get_header (nm, error);
return_if_nok (error);
}
if (!header) {
header = mono_method_get_header_checked (method, error);
return_if_nok (error);
}
/* Make modifications to a copy of imethod, copy them back inside the lock */
real_imethod = imethod;
memcpy (&tmp_imethod, imethod, sizeof (InterpMethod));
imethod = &tmp_imethod;
MONO_TIME_TRACK (mono_interp_stats.transform_time, generate (method, header, imethod, generic_context, error));

Here even if we do method = nm we haven't modified imethod->method. so even if we replaced some method by a wrapper, at execution time in interp.c we're not going to see that.

so if any of those wrappers used MINT_LDSTR_TOKEN they would have the same problem.

@BrzVlad
Copy link
Member

BrzVlad commented Jul 12, 2023

I'm pretty sure, in general, for wrapper methods imethod->method points to a wrapper MonoMethod. What exactly is the repro for this issue ?

lambdageek added a commit to lambdageek/runtime that referenced this issue Jul 12, 2023
MINT_LDSTR_DYNAMIC for dynamic methods.  the data item is an index
into the DynamicMethod:method.method_data which stores pointers to
MonoString objects that are held by the managed DynamicMethod.

MINT_LDSTR_CSTR for ilgen wrappers. the data item is a raw C string
that is global, or was allocated by malloc

Fixes dotnet#88694 by allowing
MINT_LDSTR_CSTR in any method since we no longer depend on
mono_method_get_wrapper_data at execution time.  And at transform time
we have access to the actual wrapper method.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2023
lambdageek added a commit that referenced this issue Jul 12, 2023
MINT_LDSTR_DYNAMIC for dynamic methods.  the data item is an index
into the DynamicMethod:method.method_data which stores pointers to
MonoString objects that are held by the managed DynamicMethod.

MINT_LDSTR_CSTR for ilgen wrappers. the data item is a raw C string
that is global, or was allocated by malloc

Fixes #88694 by allowing
MINT_LDSTR_CSTR in any method since we no longer depend on
mono_method_get_wrapper_data at execution time.  And at transform time
we have access to the actual wrapper method.
@ghost ghost removed in-pr There is an active PR which will close this issue when it is merged untriaged New issue has not been triaged by the area owner labels Jul 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants