Add no_std support #122

ghost · 2018-08-24T23:36:16Z

This PR should contain no breaking changes.

Add a std feature, enabled by default and sync it with parity-wasm/std
Remove uses of std::io
Change to a HashMap implementation that works on alloc
Disable std::error for no_std
The biggest change is using core and alloc everywhere and substituting std if available.
This is -- as far as I know -- the accepted way to support no_std (as mentioned in the alloc RFC).

no_std should probably be added to the Travis configuration.
Nom has a working no_std Travis: https://github.com/Geal/nom/blob/master/.travis.yml.

ghost · 2018-08-24T23:54:11Z

Also had to add a no_std feature to disable hashmap_core in no_std environments.

CryZe · 2018-08-25T00:03:41Z

Features should never / can't ever be negative. So the no_std feature is a bug waiting to happen.

ghost · 2018-08-25T00:05:39Z

How else would you enable hashmap_core only in no_std?
AFAIK, you need a feature to enable a dependency.

CryZe · 2018-08-25T00:07:17Z

Does it not work via a cfg dependency? It may actually not, because cargo has tons of bugs around features and targets.

ghost · 2018-08-25T00:30:19Z

I was not aware of cfg dependencies, thanks for the tip.
I tried it now, but sadly rust-lang/cargo#2524.

pepyakin · 2018-08-25T11:38:05Z

Hello , thanks for such a great work! I don’t have much time to review because I’m travelling now

@CryZe is right, using negative features is not recommended.

You can add a new feature ‘core’ for this though. Take a look to a cranelift for example https://github.com/CraneStation/cranelift/blob/master/lib/codegen/Cargo.toml

ghost · 2018-08-25T11:46:19Z

The "std" feature enables use of libstd. The "core" feature enables use of some minimal std-like replacement libraries. At least one of these two features need to be enabled.

This is just using different names:
With this PR, either std or no_std has to be enabled.
At Cranelift's, either std or core has to be enabled.

But sure, I can rename it, if you want me to?

pepyakin · 2018-08-25T11:52:15Z

oh sorry havent had a look into the PR. Yes, that’s should work! I think I would vote for ‘core’ because I think i’ve seen it more often used in such situations

sunfishcode · 2018-08-25T15:39:09Z

src/global.rs

-use std::rc::Rc;
-use std::cell::Cell;
+use alloc::rc::Rc;
+use core::cell::Cell;


One thing we do in Cranelift is to remap core to std in the top-level lib.rs here so that most files can just use std::. This obviously isn't necessary, but since we have some crates that support no_std and some that don't, it's nice to the code more consistent between the two.

This would have made rebasing a lot nicer, too 🙃

lexfrl · 2018-08-26T11:31:00Z

Yes, that’s should work! I think I would vote for ‘core’ because I think i’ve seen it more often used in such situations

But in Parity we use feature with name std everywhere. It was started by analogy with https://github.com/BurntSushi/byteorder/blob/master/Cargo.toml#L23

ghost · 2018-08-26T13:12:09Z

@fckt core is a negative feature, which has to be enabled iff no_std is to be used.
The default-on normal feature is still called std.
We have to use an ugly negative feature, because cfg dependencies are buggy: rust-lang/cargo#2524.

ghost · 2018-08-26T13:25:49Z

Only the float handling in value.rs is left with std dependencies.
This relies on nan-preserving-float and some bits of the standard library that are not in core (for example std::f32::fract).

I have filed a PR for no_std support in nan-preserving-float: eira-fransham/nan-preserving-float#1.
With that and libm, I was able to compile wasmi for no_std.
libm has some panics in debug mode (rust-lang/libm#4), so we still use std when available.

If the PR to nan-preserving-float is accepted and released, we're good to go.

NikVolf · 2018-08-26T19:06:56Z

Cargo.toml

+default = ["std"]
+# Disable for no_std support
+std = ["parity-wasm/std", "byteorder/std"]
+# Enable for no_std support


are those comments outdated?

No. They match the readme section about no_std.

NikVolf · 2018-08-26T19:09:30Z

src/memory.rs

+use alloc::rc::Rc;
+use core::u32;
+use core::ops::Range;
+use core::cmp;


core imports might be grouped

sorpaas · 2018-09-13T18:43:52Z

I'm trying to get this branch working with a real embedded environment (Rux microkernel!), but so far, it still needs some changes, and I would hope we can fix them!

First, hashmap_core crate doesn't work well in my environment. I hope we can remove that crate, but instead, use something like this:

#[cfg(feature = "std")]
use std::collections::{HashMap as Map};
#[cfg(not(feature = "std"))]
use alloc::btree_map::{BTreeMap as Map};

BTreeMap/BTreeSet are actually sufficient for all our use-cases, and using hashmap would impose unnecessary additional memory consumption for those embedded environments.

The second issue I'm facing is missing fract functions for floating points. That doesn't seem to always be defined in some environments, but we can do something like this in impl_float! in value.rs:

			#[cfg(not(feature = "std"))]
			fn fract(self) -> $type {
				self - self.trunc()
			}

I have a branch that's working for Rux microkernel's demo propose at https://github.com/sorpaas/wasmi/tree/jrakow-nostd. That branch also contains some further modifications that make it work with older Rust nightly versions, which I guess we don't need here.

ghost · 2018-09-14T10:09:11Z

I had actually solved the float issues locally by depending on libm for the implementation of fract.
These commits are now published here.

The remaining blocker is eira-fransham/nan-preserving-float#1, which is inactive.
I am not sure how to proceed without waiting on that PR.

sorpaas · 2018-09-14T10:12:50Z

@jrakow Would you mind also fixing the hashmap_core issue described above? We can safely use BTreeMap for no_std environments.

ghost · 2018-09-14T10:15:26Z

@sorpaas I was just writing a response to that as a second comment:

As for BTreeMap vs. hashmap_core, I am fine with using BTreeMap.
If you would open a PR against https://github.com/jrakow/wasmi/tree/no_std with your changes on that issue, I would happily merge that.

ghost · 2018-09-14T10:18:33Z

BTW, the Rust issue for float operations in libcore is rust-lang/rfcs#2505.
Using a Rust libm is the second solution highlighted there.

sorpaas · 2018-09-14T12:10:01Z

@jrakow Thanks! I still need to test whether your solutions work for me regarding libm. The issue is that I need to compile to targets that both support and does not support native floating point. I haven't tested whether libm works in all those cases. Even if it does, I think we would miss some opportunities to use native floating point ops to make code run faster -- All we're missing in libcore is the one single fract function.

Another situation is that it looks like current floating point implementations actually depend on some extern functions (which I have not yet understood why, see this).

One of the possible solutions is to add a feature flag to disable all floating points operations from compiling -- we don't need them in deterministic environments like blockchain virtual machines, so it might be a good feature to have.

Anyway, the above looks not trivial so I might just get another PR if I figured out something (and will definitely fix BTreeMap issue, but probably also just in that PR, to make pulling logistics simple!). Please feel free to continue and merge this!

pepyakin · 2018-10-20T15:53:33Z

After chatting with @Vurich in private we decided to embed nan-preserving-float into wasmi repo.

ghost · 2018-10-21T20:16:52Z

Looks like the Travis cache is getting too big.

pepyakin · 2018-10-29T10:17:31Z

Finally we merged it! Thanks for the great job, @jrakow !

* add default-enabled std feature * use parity-wasm/std feature only if std is enabled * drop dependency on std::io * use hashmap_core instead of std::collections::HashMap * disable std::error in no_std * core and alloc all the things * mention no_std in readme * add no_std feature and use hashmap_core only on no_std * rename the no_std feature to core * drop dependency on byteorder/std * simplify float impl macro * remove some trailing whitespace * use libm for float math in no_std * add note about no_std panics of libm to readme * Embed nan-preserving-float crate. * Add no_std check to the Travis CI config * add missing dev-dependency

Julius Rakow added 8 commits August 25, 2018 01:23

add default-enabled std feature

704f96e

use parity-wasm/std feature only if std is enabled

606c006

drop dependency on std::io

ffe54f9

use hashmap_core instead of std::collections::HashMap

f4173ad

disable std::error in no_std

33a0e6e

core and alloc all the things

d38d268

mention no_std in readme

3c75f65

add no_std feature and use hashmap_core only on no_std

3b82da1

rename the no_std feature to core

99eabd7

sunfishcode reviewed Aug 25, 2018

View reviewed changes

drop dependency on byteorder/std

86b1d3c

ghost mentioned this pull request Aug 26, 2018

add no_std support eira-fransham/nan-preserving-float#1

Merged

NikVolf reviewed Aug 26, 2018

View reviewed changes

src/memory.rs

use alloc::rc::Rc;

use core::u32;

use core::ops::Range;

use core::cmp;

Copy link

Contributor

NikVolf Aug 26, 2018

Choose a reason for hiding this comment

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

core imports might be grouped

Julius Rakow added 4 commits September 14, 2018 11:55

simplify float impl macro

cf67b16

remove some trailing whitespace

2a6103c

use libm for float math in no_std

0a0f870

add note about no_std panics of libm to readme

8854754

pepyakin added 2 commits October 20, 2018 17:47

Embed nan-preserving-float crate.

baa89af

Add no_std check to the Travis CI config

5a54374

add missing dev-dependency

21bff51

Merge branch 'master' into no_std

03d427b

pepyakin merged commit 20154c5 into wasmi-labs:master Oct 29, 2018

ghost deleted the no_std branch October 29, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no_std support #122

Add no_std support #122

ghost commented Aug 24, 2018

ghost commented Aug 24, 2018

CryZe commented Aug 25, 2018

ghost commented Aug 25, 2018

CryZe commented Aug 25, 2018

ghost commented Aug 25, 2018

pepyakin commented Aug 25, 2018

ghost commented Aug 25, 2018

pepyakin commented Aug 25, 2018

sunfishcode Aug 25, 2018

eira-fransham Oct 29, 2018

lexfrl commented Aug 26, 2018

ghost commented Aug 26, 2018

ghost commented Aug 26, 2018

NikVolf Aug 26, 2018

ghost Aug 26, 2018

NikVolf Aug 26, 2018

sorpaas commented Sep 13, 2018

ghost commented Sep 14, 2018

sorpaas commented Sep 14, 2018

ghost commented Sep 14, 2018

ghost commented Sep 14, 2018

sorpaas commented Sep 14, 2018

pepyakin commented Oct 20, 2018

ghost commented Oct 21, 2018

pepyakin commented Oct 29, 2018

Add no_std support #122

Add no_std support #122

Conversation

ghost commented Aug 24, 2018

ghost commented Aug 24, 2018

CryZe commented Aug 25, 2018

ghost commented Aug 25, 2018

CryZe commented Aug 25, 2018

ghost commented Aug 25, 2018

pepyakin commented Aug 25, 2018

ghost commented Aug 25, 2018

pepyakin commented Aug 25, 2018

sunfishcode Aug 25, 2018

Choose a reason for hiding this comment

eira-fransham Oct 29, 2018

Choose a reason for hiding this comment

lexfrl commented Aug 26, 2018

ghost commented Aug 26, 2018

ghost commented Aug 26, 2018

NikVolf Aug 26, 2018

Choose a reason for hiding this comment

ghost Aug 26, 2018

Choose a reason for hiding this comment

NikVolf Aug 26, 2018

Choose a reason for hiding this comment

sorpaas commented Sep 13, 2018

ghost commented Sep 14, 2018

sorpaas commented Sep 14, 2018

ghost commented Sep 14, 2018

ghost commented Sep 14, 2018

sorpaas commented Sep 14, 2018

pepyakin commented Oct 20, 2018

ghost commented Oct 21, 2018

pepyakin commented Oct 29, 2018