Add no_std compatibility

[SzymonKubica]

What was added?

Added support for rbpf to run on no_std environments.

Why is it needed?

Using rbpf on targets without std support increases compatibility of the crate.
I've been using a modified version of rbpf to run eBPF programs on microcontrollers (see [here(https://github.com/SzymonKubica/mibpf))

Testing

Currently the code builds successfuly with both configuration options enabled, i.e. all build configurations succeed:

cargo build
cargo build --features cranelift
cargo build --no-default-features
cargo build --features cranelift --no-default-features

All tests (except for the ones using the print - only available in std) pass in both configuration options.

TODO items for the PR

• test no_std on an example embedded device

Notes

The structure of std / no_std feature-gating is inspired by serde, inside of lib.rs a public module: lib is defined and it
contains reexports of all required items from std/alloc/core. It is then used throughout the codebase by importing all items
declared inside of it:

use crate::lib::*;

This behaves similarly as if we were using the standard library as we don't have to import things like String or Vec manually.

[qmonnet]

Hi, and thanks!
Sorry for the delay, I meant to take a look last weekend but this slipped out of my mind 😬. I'll probably need a few more days before I have time to look into the details of your PR.

In the meantime, would you mind fixing the issue raised by the Appveyor workflow please (Windows)? Given that you use config(jit) instead of Windows-related configuration options, we no longer disable the JIT stuff on Windows, causing the tests to try to run with the JIT and fail on Windows.

Could you please also sign-off your commits?

[SzymonKubica]

Hi, thank you for looking into the PR. Please don't worry about the timeline / delays, I realise that it is a non-trivial change and requires time to actually look through all of this.

I will look into the appveyor issue on windows and make sure it gets fixed. I will also start signing off the commits from now on.
Thanks

[qmonnet]

Apologies for the delay, I've kept this PR in a corner of my mind but struggled to find the time (and motivation) to dive into it.

Here are some elements of feedback, I see that the PR is indeed still in progress and needs some polishing. See also inline below.

We'd need specific jobs in the CI workflow as well, to make sure that we build both with and without the standard library to test your changes and detect regressions.

[SzymonKubica]

Hi,
Thank you for reviewing the PR and listing the required changes.
I will start going through these and clarify in the comments above if anything turns out to be unclear.

No worries about the delay, my goal with this PR was to contribute back a small subset of changes that I needed to introduce to
rbpf for my project. I've been working on a heavily-modified fork for a while and added an ARMv7 JIT to it. Given the scope of those changes and the fact that they are coupled with my specific requirements, I don't think I will be trying to make them mergeable with the upstream rbpf (although if you want to have a look it's here)

I think compared to those changes, adding no_std support is potentially useful so I will continue working on this PR

[SzymonKubica]

One note regarding your comment about asm_parser and disassembler being disabled:
I have looked into those and I don't see anything which would prevent them from running under no_std. I will update those so that they don't have to be disabled. It was probably an oversight on my part as I was primarily interested in using the VM.

[SzymonKubica]

Ok I did some restructuring and now the import switching is done the same way serde does it.
I also changed some dependencies and now all modules build with both std/no_std.
I will look into the jit feature and see if I can make it work with windows (it could be that I can make the JIT enabled in no_std, in which case I could simply revert to using the windows feature flag)

[qmonnet]

Can you please sign-off your commits?

[SzymonKubica]

Re: signing off commits
Yes I will do that from now on, I think some of my previous commits were already signed-off, but I noticed that the build doesn't go through because the old ones weren't signed-off. I will apply the clean changes by signing off commits and force push the update

[SzymonKubica]

Ok @qmonnet I think the PR is in much better state now. Following your advice I have split my changes into two logical commits:

• adding the "std" feature and modifying the feature-gating for the jit to require that both not(windows) and feature = "std" are set.
• adding the proper reexports for no_std
  I updated the description of the PR, now everything builds correctly and all available tests pass in both std and no_std.

I have also tested that it builds correctly on an actual no_std target: STM32-f439zi running RIOT. I managed to flash this example program:

#![no_std]

use riot_wrappers::riot_main;
use riot_wrappers::println;
use riot_wrappers::riot_sys::malloc;

extern crate rust_riotmodules;
extern crate rbpf;
extern crate alloc;

riot_main!(main);

fn main() {
    let prog = &[
         0x79, 0x11, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, // Load mem from mbuff into R1.
         0x69, 0x10, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, // ldhx r1[2], r0
         0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00  // exit
     ];
     let mem = &mut [
         0xaa, 0xbb, 0x11, 0x22, 0xcc, 0xdd
     ];

     // Just for the example we create our metadata buffer from scratch, and we store the
     // pointers to packet data start and end in it.
     let mut mbuff = [0u8; 32];
     unsafe {
         let mut data     = mbuff.as_ptr().offset(8)  as *mut u64;
         let mut data_end = mbuff.as_ptr().offset(24) as *mut u64;
         *data     = mem.as_ptr() as u64;
         *data_end = mem.as_ptr() as u64 + mem.len() as u64;
     }

     // Instantiate a VM.
     let mut vm = rbpf::EbpfVmMbuff::new(Some(prog)).unwrap();

     // Provide both a reference to the packet data, and to the metadata buffer.
     let res = vm.execute_program(mem, &mut mbuff).unwrap();
     assert_eq!(res, 0x2211);
     println!("res: 0x{:x}", res);
}

It looks like everything works in no_std. One note: I ended up having to disable the assembler and asm_parser as they depend on the combine crate which doesn't have the no_std mode.

I can quickly publish a repo with the experimental setup that I used to test the new version of rbpf running without std in RIOT if you would find this helpful.

[qmonnet]

Looks much better, thanks! Some comments here and there.

[qmonnet]

Please clean up the commit log from your second commit, you have multiple sign-off tags (likely because you squashed commits together and kept the descriptions of all commits).

[qmonnet]

Thanks a lot! Sorry I didn't have the time to review last night, I'll do it soon.

[SzymonKubica]

Thanks a lot! Sorry I didn't have the time to review last night, I'll do it soon.

No worries at all, take your time. No need to rush, I realise it is quite a big change in terms of files affected (not that big functionality-wise). Thank you for your time and guidance with the PR.

[qmonnet]

Thanks a lot for your work!

I've got some comments, and I think I'm a bit confused as to when the assembler is supported without std. I probably need to take some time to play with your code and check why rbpf complains if I try to enable the tests; in the meantime, I commented at the locations where it is not clear to me.

The required changes are mostly minor things, we're getting closer!

Regarding the formatting of your patches, I'd recommend the following:

• Squash the fixes from the last commit into the relevant earlier commits
• Rework your commit descriptions, in particular for the second commit. What's interesting in this description is not so much to describe the changes (and not the history of the different changes and fixes incorporated during the review); instead, it should provide the motivation for the change, and explain what the commit addresses, what difficulties arose and how the commit works around them, etc. Everything related to the context of the change.

[SzymonKubica]

Thanks a lot for your work!

I've got some comments, and I think I'm a bit confused as to when the assembler is supported without std. I probably need to take some time to play with your code and check why rbpf complains if I try to enable the tests; in the meantime, I commented at the locations where it is not clear to me.

The required changes are mostly minor things, we're getting closer!

Regarding the formatting of your patches, I'd recommend the following:

• Squash the fixes from the last commit into the relevant earlier commits
• Rework your commit descriptions, in particular for the second commit. What's interesting in this description is not so much to describe the changes (and not the history of the different changes and fixes incorporated during the review); instead, it should provide the motivation for the change, and explain what the commit addresses, what difficulties arose and how the commit works around them, etc. Everything related to the context of the change.

Ok thank you for the review and your suggestions. I will rework the commit messages and squash the subsequent fixes.
I propose the following logical changes:

• add the std feature and updating the windows cfg flags
• add lib module and conditional imports (the Configure combine crate and asm_parser to work under no_std. commit will be squashed into this one)
• update readme with feature description
• add workflow job

The last commit fixing pipeline warnings will be split and squashed into the above commits where the changes belong appropriately

[SzymonKubica]

Ok I think everything has been cleaned up now. Thank you very much for all of your feedback and time spent reviewing my changes. Now all imports / dependency changes are in the second commit. I managed to find some additional assembler tests that could be reenabled (tests/misc.rs). I think it helps a lot to see all changes in a single logical commit

[SzymonKubica]

@qmonnet could you please approve the CI workflow run? Don't worry about reviewing the changes today, no need to rush. I just wanted to make sure that rearranging the changes didn't produce any new pipeline warnings. Enjoy your weekend and feel free to look into the PR when you are free.

[SzymonKubica]

Hi @qmonnet, thank you for triggering the workload, seems like the pipeline passes successfully and no new warnings emitted.
I have addressed all of your comments and the commit descriptions have been revised.
Please let me know if anything else needs to be changed.

[qmonnet]

Apologies once again for the delay. I do realise this is not a great way to keep you motivated for this PR, and I'm sorry for the poor contribution experience in terms of delays. Thanks a lot for bearing with me.

And thanks for this work, this is in a very nice shape now! I think there's just a small fix to do in the README, but everything else looks good as far as I can tell. Well done, and nice work on the commit description for your second commit! ❤️

[SzymonKubica]

Apologies once again for the delay. I do realise this is not a great way to keep you motivated for this PR, and I'm sorry for the poor contribution experience in terms of delays. Thanks a lot for bearing with me.

And thanks for this work, this is in a very nice shape now! I think there's just a small fix to do in the README, but everything else looks good as far as I can tell. Well done, and nice work on the commit description for your second commit! ❤️

Ok @qmonnet, thank you for pointing out the issue it the README, it is fixed now. Thank you for picking up on it.

[qmonnet]

Awesome, thanks a lot for this work!

[SzymonKubica]

Perfect, thank you for approving the PR 🚀
I really appreciate your help along the way. It was a great learning experience and I think it is going to be very useful for my future open-source contributions. Thanks for your time and being open to this contribution.

[qmonnet]

Congrats on getting your µBPF paper into the ACM workshop! 🎉

[SzymonKubica]

Congrats on getting your µBPF paper into the ACM workshop! 🎉

Thank you!