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

feat(ext/ffi): Implement UnsafePointer and UnsafePointerView API #12828

Merged
merged 26 commits into from
Dec 15, 2021

Conversation

eliassjogreen
Copy link
Contributor

@eliassjogreen eliassjogreen commented Nov 20, 2021

This pr adds support for passing, returning and reading pointers as proposed by the UnsafePointer and UnsafePointerView api (#12824).

Closes #12809, #12456, #12824 and #12752

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2021

CLA assistant check
All committers have signed the CLA.

@AaronO AaronO self-requested a review November 20, 2021 19:09
@eliassjogreen
Copy link
Contributor Author

This pr could very easily implement UnsafePointer as proposed in #12824 if that is what we want. It would most likely make working with pointers way easier imo

@bnoordhuis
Copy link
Contributor

I'm not sure using f64's is sound. V8 tends to canonicalize NaNs - maybe Infs and subnormals too, but definitely NaNs - which means there's a subset of the 64 bit space that's going to get corrupted.

You could maybe get away with it if it's only NaNs and Infs1 but definitely not when it's subnormals too. An example of a subnormal is 0000 0000 0000 0001 (1u64 interpreted as f64.)

tl;dr Don't rely on V8 leaving the top 12 bits untouched, use BigInts instead.

1 ARM64 and x86_64 have up to 52 address lines, which is exactly the f64 significand's size. NaNs and Infs are encoded in the exponent, not the significand.

@eliassjogreen eliassjogreen changed the title feat(ext/ffi): returning and passing pointers transmuted as f64 feat(ext/ffi): Implement UnsafePointer API Nov 22, 2021
@eliassjogreen
Copy link
Contributor Author

@bnoordhuis internally pointers are now represented as the high and low bytes of an u64 ((u32, u32)) which should avoid the issues with transmuting pointers as f64. I also implemented the UnsafePointer API to make it easier to work with pointers although it should probably be looked through for my sanity and to make sure it all works as expected, cc @DjDeveloperr

Copy link
Contributor

@DjDeveloperr DjDeveloperr left a comment

Choose a reason for hiding this comment

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

LGTM, got a few comments

ext/ffi/00_ffi.js Outdated Show resolved Hide resolved
ext/ffi/00_ffi.js Outdated Show resolved Hide resolved
ext/ffi/lib.rs Outdated Show resolved Hide resolved
@eliassjogreen eliassjogreen changed the title feat(ext/ffi): Implement UnsafePointer API feat(ext/ffi): Implement UnsafePointer and UnsafePointerView API Nov 29, 2021
@eliassjogreen
Copy link
Contributor Author

This PR is ready for review now!

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Nice work. Mostly LGTM.

ext/ffi/lib.rs Outdated Show resolved Hide resolved
ext/ffi/lib.rs Outdated Show resolved Hide resolved
ext/ffi/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks Elias, LGTM.

@brandonros
Copy link

brandonros commented Dec 15, 2021

What next release can we expect this to land in? v1.17.0? When is that expected to land?

https://github.com/denoland/deno/milestone/28 currently says "past due 1 day" as of 2021-12-15

excited for this to land!!!

@DjDeveloperr
Copy link
Contributor

DjDeveloperr commented Dec 15, 2021

Yes, it's going to be in 1.17.0 release which is a bit delayed it seems. Can expect it in a day or two I think

@lucacasonato
Copy link
Member

@DjDeveloperr It is not delayed. 1.17.0 will release tomorrow as documented in the release schedule: https://deno.land/manual@v1.16.4/contributing/release_schedule. I have updated the milestone accordingly now.

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.

feature request: support for null value in FFI buffer argument
8 participants