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] Fix GetFunctionPointer #13708

Merged
merged 1 commit into from Apr 4, 2019
Merged

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Mar 28, 2019

This method returns a function pointer that can be called with a calli instruction. On interpreter we use a pointer to InterpMethod while on jit we use the native code address. Normally, GetFunctionPointer should return the InterpMethod pointer if called from interp or the native code address if called from jit. Since we don't have any information about the execution engine of the caller, we solve this by intrinsifying all these calls, that happen in the interpreter.

Passing such function pointers between jitted and interp code is probably still unreliable.

Fixes #13654

@BrzVlad BrzVlad requested a review from lewurm as a code owner March 28, 2019 10:41
@BrzVlad
Copy link
Member Author

BrzVlad commented Mar 29, 2019

@monojenkins build failed

This method returns a function pointer that can be called with a calli instruction. On interpreter we use a pointer to InterpMethod while on jit we use the native code address. Normally, GetFunctionPointer should return the InterpMethod pointer if called from interp or the native code address if called from jit. Since we don't have any information about the execution engine of the caller, we solve this by intrinsifying all these calls, that happen in the interpreter.

Passing such function pointers between jitted and interp code is probably still unreliable.

Fixes mono#13654
@lewurm
Copy link
Contributor

lewurm commented Apr 2, 2019

let's wait with merging until @steveisok confirmed the fix

@steveisok
Copy link
Contributor

I applied the fix and ran code I supplied in the original issue:

https://gist.github.com/steveisok/a04557c332dfaf6ba5415a6b426ec39d

It still results in a crash as detailed below:

https://gist.github.com/steveisok/858c757e33596d6ccf874af6b82a379d

I haven't had the chance to dig deeper yet.

@BrzVlad
Copy link
Member Author

BrzVlad commented Apr 4, 2019

@steveisok Are you sure you applied the fix correctly. For me, this change fixes the crash on an amd64 linux with mono master. What platform are you running it on ?

@steveisok
Copy link
Contributor

Ugh - I ran it on my mac :-). Linux is what we test the interpreter on, right?

I confirmed it works on linux - GTG.

@BrzVlad
Copy link
Member Author

BrzVlad commented Apr 4, 2019

@steveisok I think we test on linux just because it's easy to get the machines, but it should also work on macos. I tried it locally on my mac and it was also fixing the issue.

@lewurm lewurm merged commit 474305c into mono:master Apr 4, 2019
@marek-safar
Copy link
Member

@monojenkins backport 2019-04

@marek-safar
Copy link
Member

@monojenkins backport 2019-02

@monojenkins
Copy link
Contributor

@marek-safar backporting to 2019-04 failed, the patch results in conflicts:

Applying: [interp] Fix GetFunctionPointer
Using index info to reconstruct a base tree...
M	mono/mini/interp/interp.c
M	mono/mini/interp/mintops.def
M	mono/mini/interp/transform.c
Falling back to patching base and 3-way merge...
Auto-merging mono/mini/interp/transform.c
CONFLICT (content): Merge conflict in mono/mini/interp/transform.c
Auto-merging mono/mini/interp/mintops.def
Auto-merging mono/mini/interp/interp.c
error: Failed to merge in the changes.
Patch failed at 0001 [interp] Fix GetFunctionPointer

Please backport manually!

@monojenkins
Copy link
Contributor

@marek-safar backporting to 2019-02 failed, the patch results in conflicts:

Applying: [interp] Fix GetFunctionPointer
Using index info to reconstruct a base tree...
M	mono/mini/interp/interp.c
M	mono/mini/interp/mintops.def
M	mono/mini/interp/transform.c
Falling back to patching base and 3-way merge...
Auto-merging mono/mini/interp/transform.c
CONFLICT (content): Merge conflict in mono/mini/interp/transform.c
Auto-merging mono/mini/interp/mintops.def
Auto-merging mono/mini/interp/interp.c
error: Failed to merge in the changes.
Patch failed at 0001 [interp] Fix GetFunctionPointer

Please backport manually!

alexanderkyte pushed a commit to alexanderkyte/mono that referenced this pull request May 6, 2019
This method returns a function pointer that can be called with a calli instruction. On interpreter we use a pointer to InterpMethod while on jit we use the native code address. Normally, GetFunctionPointer should return the InterpMethod pointer if called from interp or the native code address if called from jit. Since we don't have any information about the execution engine of the caller, we solve this by intrinsifying all these calls, that happen in the interpreter.

Passing such function pointers between jitted and interp code is probably still unreliable.

Fixes mono#13654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interpreter Crash When Doing Fancy Reflection
5 participants