Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Support u128/i128 in runtime interface #4703

Merged
merged 2 commits into from
Jan 22, 2020
Merged

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Jan 21, 2020

This implements support for u128/i128 as parameters/return value in
runtime interfaces. As we can not pass them as identity, as for the
other primitives types, we pass them as an pointer to an [u8; 16] array.

This implements support for `u128`/`i128` as parameters/return value in
runtime interfaces. As we can not pass them as identity, as for the
other primitives types, we pass them as an pointer to an `[u8; 16]` array.
@bkchr bkchr added the A0-please_review Pull request needs code review. label Jan 21, 2020
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Thank you!

@bkchr bkchr requested a review from pepyakin January 21, 2020 21:36
let data = context.read_memory(Pointer::new(arg), 16)?;
let mut res = [0u8; 16];
res.copy_from_slice(&data);
Ok(unsafe { mem::transmute::<[u8; 16], $type>(res) })
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I wonder if this is safe? The alignment of u128 might be 16 and I am not sure sure if we can say anything regarding this about res.
  2. Does it assume that wasm and host have the same endianness?

I think this can be swapped with u128::from_le_bytes here instead of transmute?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. What? Why can we not say the same about res? It is as well 16 bytes. (Looking at the rust source code for from_le_bytes it does exactly the same transmute)
  2. Yes it assumes it. Please don't start again this xD As it is no primitive type, I probably also need to call to_le_bytes on the host?

Copy link
Member Author

Choose a reason for hiding this comment

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

But good catches! I searched yesterday evening for *_let_bytes, but failed for some reason 🤷‍♀️

@bkchr bkchr merged commit ef578cd into master Jan 22, 2020
@bkchr bkchr deleted the bkchr-u128-i128-runtime-interface branch January 22, 2020 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants