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: no_std support #12

Merged
merged 18 commits into from
Oct 13, 2019
Merged

feat: no_std support #12

merged 18 commits into from
Oct 13, 2019

Conversation

roblabla
Copy link

@roblabla roblabla commented Aug 17, 2019

Blocking on servo/rust-smallvec#160

Fixes #11

Was easier than I thought it would be 👀

@roblabla roblabla changed the title no_std support feat: no_std support Aug 17, 2019
@dignifiedquire
Copy link
Owner

Wow, that is a very small change, did not expect that. I guess it was good I switched to SmallVec afterall

@roblabla
Copy link
Author

So I did a quick test, and turns out testing with no_std isn't enough, we need to build for a target that actually has no std crate, otherwise it might still get implicitly linked...

So I tried compiling for an embedded arm target (thumbv7m-none-eabi), Which uncovered a problem: In libcore, we're missing some floating point functions. Specifically:

  • log2
  • ln
  • sqrt
  • cbrt

I also hit another problem trying to compile for an embedded arm target: I think the build.rs probe function is broken in cross-compiling scenario? It doesn't seem to be passing the target through. We should probably be using autocfg, which takes care of this sort of thing for us.

@dignifiedquire
Copy link
Owner

It doesn't seem to be passing the target through. We should probably be using autocfg, which takes care of this sort of thing for us.

yeah agreed, I inherited that build.rs when I forked this project

@dignifiedquire
Copy link
Owner

Which uncovered a problem: In libcore, we're missing some floating point functions.

Fun, do you think it makes sense to backfill them in here, or to get them in upstream?

@roblabla
Copy link
Author

log2 seems to mostly be used to get the bitlength of a number in various places. It can be replaced with a log2_ceil integer operation (and heck, it might even be a little faster):

fn log2(x: u32) -> u32 {
    assert!(x > 0);
    num_bits::<u32>() as u32 - x.leading_zeros() - 1
}

ln is used in nth_root, sqrt is used in sqrt, and cbrt is used in cbrt. For those, I guess there's two choices:

  1. we use https://docs.rs/libm/0.1.4/libm/ (which is used by compiler-builtins, it's part of the core rust infrastructure) to provide an implementation of those functions
  2. We can just cfg-out the functions in core targets. (I don't think they're used by RSA?)

@dignifiedquire
Copy link
Owner

Lets pull in libm for the no_std target, I think it is great to keep full support for these things and makes life easier in the long run.

@roblabla
Copy link
Author

ugh. I'm hitting this stupid cargo bug again: rust-lang/cargo#4866

Specifically, criterion is activating the std feature of num-trait. I'm unsure how to proceed.

@roblabla
Copy link
Author

I pushed everything I have so far. What's missing:

The later will need a bit of investigation, I've got zero clue what is going on there. num-integer implements u128 when they detect the presence of u128 in exactly the same way we do...

@roblabla
Copy link
Author

I moved the benchmark to a separate crate, fixing bullet point 1. As for the second bullet point... I can't reproduce it anymore? I rm -rf'd my target folder and it's gone. Spooky.

So now all we have to do is wait for smallvec and it should be good to merge.

@dignifiedquire
Copy link
Owner

dignifiedquire commented Aug 18, 2019

can you add some notes to the readme on using the no_std version please?

@roblabla
Copy link
Author

Sure! Should I also add some information in the lib.rs module documentation?

@dignifiedquire
Copy link
Owner

dignifiedquire commented Aug 18, 2019 via email

@dignifiedquire
Copy link
Owner

@roblabla it looks like a smallvec update is still not in sight. How about we change this such that using no_std requires nightly for now, so we can merge and move forward with this?

@roblabla
Copy link
Author

roblabla commented Sep 26, 2019

It already works on nightly (just had a small broken test, last commit should fix it). It will fail to build on stable with a confusing error (as seen on CI) until smallvec publishes an update.

Should I update the CI so that it only runs no_std build on nightly for now?

@dignifiedquire
Copy link
Owner

Should I update the CI so that it only runs no_std build on nightly for now?

Yes please, and add a note about it to the docs. Then I'll merge and publish this, and we can update it whenever smallvec does a new release

@roblabla
Copy link
Author

"A small broken test" might have been an underestimation lol (most tests required minor tweaks). This should be good to go for merging now.

@dignifiedquire dignifiedquire merged commit 874ab0f into dignifiedquire:master Oct 13, 2019
@dignifiedquire
Copy link
Owner

Thank you for all the work on this!

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.

no_std support
2 participants