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

Solving issues in https://github.com/saghul/txiki.js/issues/653 #654

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KaruroChori
Copy link
Contributor

This solves some issues I found out while debugging #653.
The current implementation of strings as an advanced type is extremely problematic.
What is really passed onto the C function is a clone of the original object with a shorter lifetime due to its scoping. This causes issues when that object is expected to be consumed later once the reference counter already collected it.

The new syntax is not as compact or nice, but there is no reasonable way to make the original better from what I can tell (without hacking the js syntax).
I already ported these changes to the test-suite and verified it worked.

There is one missing bit, at the moment fromBuffer for CString is very wasteful as it forces to decode and reencode.
There is surely a better way to bind the buffer directly, but I am not very familiar with the intrinsics provided in txiki.

@saghul
Copy link
Owner

saghul commented Sep 22, 2024

@lal12 mine taking a look?

@lal12
Copy link
Contributor

lal12 commented Sep 23, 2024

I don't think it is that big of an issue. It is not quite obvious what happens, but that if a C function expects the memory to be existent for a while you have to take special care, that is just an inherent thing with memory unsafe programming languages.

It also isn't quite "unreasonable" to expect such a behavior, since strings are primitives in JS and therefore effectively call-by-value.

This PR would be additional overhead and less comfortable for most use cases.

There can an argument be made for the automatic behavior of the parser though, since it cannot really decide what type to use (advanced buffer, string or simple ptr). I never really intended it to be a full C prototype parser, just for an easier way for people to use ffi without having to learn an API, so you can control it by using void* instead of char*, e.g.. This of course isn't really applicable when using it with original header files.

@lal12
Copy link
Contributor

lal12 commented Sep 23, 2024

I guess allowing multiple things a string, a buffer, a pointer (or even a dedicated ffi string class) to be passed, would be an option though.

@KaruroChori
Copy link
Contributor Author

KaruroChori commented Sep 23, 2024

I don't think it is that big of an issue. It is not quite obvious what happens, but that if a C function expects the memory to be existent for a while you have to take special care, that is just an inherent thing with memory unsafe programming languages.

While I agree in principle, as this would apply to pointers and buffers alike, for string it is just made worse by the attempt of a cleaner syntax.
The current implementation is hiding ctx.buf = (new TextEncoder()).encode(str+'\0'); on a narrower scope compared to the lifetime of the original string, and it will never be possible to keep it alive from the outside since its reference is lost shortly after. Because of that, strings are intrinsically more broken compared to basic pointers, and the user would not even know why if they don't go and check the implementation in txiki:ffi.
If the lifetime is fine and can be controlled for everything but not for strings I would consider it a problem of the string implementation. As they are implemented there is just no way to make a safe-ish use of them.

@lal12
Copy link
Contributor

lal12 commented Sep 23, 2024

Some information about this should obviously go into the documentation.

I am not fundamentally in disagreement, I guess it really depends on the use case, what in the end is more practical.

But I just noticed another issue in the current implementation and the PR. If you get a char* pointer as a return and need it later (e.g. for passing it to a free function), it also would be lost.

@saghul
Copy link
Owner

saghul commented Sep 24, 2024

What do other ffi APIs do here? For example Python's cffi? Perhaps we could borrow some inspiration from there.

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.

3 participants