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

Add ARMv7 as CI test target #186

Merged
merged 2 commits into from Jun 18, 2019
Merged

Add ARMv7 as CI test target #186

merged 2 commits into from Jun 18, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jun 10, 2019

This adds ARMv7 as test target for Travis CI so that issues on this 32-bit platform can be spotted immediately which should also resolve #43.

To do so, I opted to bump the CI to Xenial to avoid needing third-party repositories as everything just works using crossbuild-essential-armhf and qemu-user-static on this platform.

I also to install packages using sudo apt-get instead of the APT addon as the latter does not support conditional installs based on the build matrix, but I did not want to use a different installation method of the conditional and the unconditional dependencies.

Note that the spec tests currently fail due to failures in wasm_conversions. I would be glad if someone could help me with fixing those.

@parity-cla-bot

This comment has been minimized.

@pepyakin pepyakin self-requested a review June 10, 2019 13:14
@ghost

This comment has been minimized.

@parity-cla-bot
Copy link

It looks like @adam-rhebo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@pepyakin
Copy link
Collaborator

Ok, this turned out to be trickier that I initially thought.

The test that fails is this one.

It tests i32.trunc_f32_s instruction,

  • here is the general conversion instruction description.
  • here is the specific operator description.

This is the implementation of the opcode and it looks like the problem is in this code:

https://github.com/paritytech/wasmi/blob/284c907b29a358d8345c1884dcbfb3e3b661185f/src/value.rs#L372-L385

The comment mentions that it is UB to convert value such as Inf or NaN. However, it also looks like that it is UB to convert a value that is outside of range of the target type, see rust-lang/rust#10184.

I briefly tried to fix it by adding explicit bounds check but it didn't help. I guess there is a chance that something else might be going on, but I don't have ARM hardware nearby nor don't have a lot of time to look into it. If you decide to fix this and bump into problems feel free to ask me questions.

@ghost
Copy link
Author

ghost commented Jun 13, 2019

@pepyakin I had a look and I think anything that tries to detect the failed conversions using self as $into will not work as the UB already happened at that point. Hence I pushed a variant that uses arbitrary precision numbers from the num crate to perform the conversion which surely has worse performance, but does yield correct results. This can probably go away again when the upstream issue is resolved.

I am not completely sure, but I guess the same could be applied to impl_wrap_into!(f64, f32);?

@ghost
Copy link
Author

ghost commented Jun 13, 2019

@pepyakin I had to check the cross-compiled tests to use --release builds as they would hit the Travis CI build timeouts otherwhise. Normally, this would be unfortunate as it makes for less useful stack traces, but since the tests run under qemu-arm-static, the stack traces are already not very useful...

test.sh Show resolved Hide resolved
test.sh Show resolved Hide resolved
Copy link
Collaborator

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

But otherwise seems good!

UPD: let me expand a bit. Since:

  1. this project more values correctness over performance
  2. tries to let people experiment with wider range of platforms
  3. use cases of paritytech don't require floats

I am inclined to trade off some performance in order to support more platforms. If anybody comes up with a better way to do this I'd be happy to merge it.

When truncating floating point values to integer values, we need to
avoid undefined behavior if the argument does not fit into the target
type which is currently impossible using casts of primitive types.

Hence, this reimplements those conversions using arbitrary precision
integers and rationals from the num crate.
@ghost
Copy link
Author

ghost commented Jun 18, 2019

If anybody comes up with a better way to do this I'd be happy to merge it.

Ideally, rustc will be improved after LLVM has the necessary intrinsics and we will get things like impl TryFrom<f32> for i32 which we can then just use here instead of the arbitrary precision numbers.

@pepyakin pepyakin merged commit 7fe6ef4 into wasmi-labs:master Jun 18, 2019
@pepyakin
Copy link
Collaborator

Yeah.

But TBH I am still not sure why checking the range before casting doesn't work (as in my tentative fix). I still think that it might be possible to solve it this way.

@ghost ghost deleted the armv7-travis-ci branch June 18, 2019 14:09
@ghost
Copy link
Author

ghost commented Jun 18, 2019

But TBH I am still not sure why checking the range before casting doesn't work (as in my tentative fix). I still think that it might be possible to solve it this way.

From my debugging, I saw that the value actually was in range, i.e. for x86-64 and arm, self.truncate() <= $into::max_value() as $from was always true. On x86-64, however, self.truncate() had a different sign bit from (self as $into) as $from which did not held on arm. This is completely "valid" as self as $into is already UB.

mystor added a commit to mystor/wasmi that referenced this pull request Feb 17, 2020
Two issues are fixed:

 1. num-bigint 0.2 requires std, and is used to perform float to int conversions
    since wasmi-labs#186. This patch disables this change in no_std environments.
    This change can be reverted once num-bigint 0.3 is released, which will
    support no_std.

 2. The `f{32,64}::fract` method is currently not implemented by core, only std.
    This patch uses the no_std supporting implementation from num-trait's
    FloatCore trait.
@mystor mystor mentioned this pull request Feb 18, 2020
mystor added a commit to mystor/wasmi that referenced this pull request Feb 18, 2020
Two issues are fixed:

 1. num-bigint 0.2 requires std, and is used to perform float to int conversions
    since wasmi-labs#186. This patch disables this change in no_std environments.
    This change can be reverted once num-bigint 0.3 is released, which will
    support no_std.

 2. The `f{32,64}::fract` method is currently not implemented by core, only std.
    This patch uses the no_std supporting implementation from num-trait's
    FloatCore trait.
pepyakin pushed a commit that referenced this pull request Feb 18, 2020
* Fix wasmi no_std support (#218)

Two issues are fixed:

 1. num-bigint 0.2 requires std, and is used to perform float to int conversions
    since #186. This patch disables this change in no_std environments.
    This change can be reverted once num-bigint 0.3 is released, which will
    support no_std.

 2. The `f{32,64}::fract` method is currently not implemented by core, only std.
    This patch uses the no_std supporting implementation from num-trait's
    FloatCore trait.

* Ensure wasmi builds without std in CI

The no-std-check cargo subcommand uses a custom sysroot to prevent the crate
from using std while checking a local target. This should help ensure that
changes which introduce std dependencies are caught at review time.
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.

Support 32-bit targets
2 participants