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

P/Invoke fixes #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

memsom
Copy link

@memsom memsom commented Sep 8, 2017

This makes the P/Invoke interface work again.

No idea is this will be useful, but it really does put things back to working in a pure DNA built on to run on the command line.

I think that adding in the extra params was the main mistake. This makes the P/Invoke fall in to a little heap on the floor. I think making an extra attribute or calling convention for the C# side would be a better method for getting this working. More metadata would allow you to automagically add the extra params when they are required.

This makes the P/Invoke interface work again.

No idea is this will be useful, but it really does put things back to working in a pure DNA built on to run on the command line.

I thin that adding in the extra params was the main mistake. This makes the P/Invoke fall in to a little heap on the floor. I think making an extra attribute or calling convention for the C# sire would be a better method for gettig this working.
@memsom
Copy link
Author

memsom commented Sep 8, 2017

I had a look and libffi and dyncall are both capable of adding in a dynamic interface for calling functions without having a big heap of pre-defined function pointers. Would one of those work for you?

@davidfowl
Copy link
Collaborator

What's the purpose of this change?

@SteveSanderson
Copy link
Owner

If I understand correctly, this is to reintroduce some wider p/invoke functionality specifically for the "running as native code, not under wasm/asm.js" case.

If that is correct, then although I appreciate the contribution @memsom, I don't think we'd want to maintain any code here that's for the non-wasm/asm.js case. The goal in this repo isn't to maintain a general .NET runtime that targets multiple platforms, but rather only one that targets wasm/asm.js.

@memsom
Copy link
Author

memsom commented Sep 21, 2017

Hey guys, fair enough. I contributed it back as I wholesale took you updates for .Net Core and felt I could give back to you a very simple fix to the comment @SteveSanderson had made about possibly working out what broke. You have a chicken and egg issue - you added fixed params to the method signature, so the compute of the params is completely broken. That was the main issue. As I said, you don't need to do that to make a generic P/Invoke that can take any arbitrary params. You just need not to set it up that way in the first instance. It's your project, you get to pick what you want it to accept. ;-)

Oh - and no, not necessarily true: "running as native code, not under wasm/asm.js". If you use this patch, and craft your wasm API interface to respect the format of the P/Invoke interface, rather than changing the P/Invoke format to fit the pattern of the calls you need - you'll be able to add any arbitrary calls in to your API as you indicated you could possibly want to do in the future.

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.

4 participants