Add no_std compatibility

SzymonKubica commented 7 months ago

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

[x] 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 commented 7 months ago

Hi, and thanks! Sorry for the delay, I meant to take a look last weekend but this slipped out of my mind :grimacing:. 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 commented 7 months ago

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

SzymonKubica commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Can you please sign-off your commits?

SzymonKubica commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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).

SzymonKubica commented 4 months ago

Ok I think all of the comments have been addressed. Thank you very much for the feedback. Key changes introduced:

now examples are configured inside Cargo.toml and the ones that need std are disabled when testing on no_std
the misc, ubpf_verfier, ubpf_vm tests now run all available tests under no_std (no more coarse-grained file level feature-gating) Thank you for picking up on this issue.
removed whitespace and formatting issues that were introduced by mistake
reordered the imports as you suggested
added required explanation / clarification comments

SzymonKubica commented 4 months ago

Hi @qmonnet, I added a description of the no_std feature to the README, it was based on how serde explains their implementation of no_std support. This was done to resolve one of your eariler comments that it isn't obvious what is available and what isn't under no_std. I think this was the last comment that was left to be addressed, so feel free to have another look at the PR when you're available. Thank you for all of your feedback, it was really helpful to learn how to prepare a patch properly.

qmonnet commented 4 months ago

It could be interesting to add the following change as an additional commit to your PR:

diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
index 60eefc28d727..a11391c430ca 100644
--- a/.github/workflows/test.yaml
+++ b/.github/workflows/test.yaml
@@ -17,10 +17,11 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
+        toolchain: [stable, beta, nightly]
+        features: [--all-features]
         include:
-          - toolchain: stable
-          - toolchain: beta
           - toolchain: nightly
+            features: --no-default-features

     steps:
       - name: Checkout code
@@ -33,12 +34,12 @@ jobs:
           override: true
           components: clippy

-      - name: Build with ${{ matrix.toolchain }}
+      - name: Build with ${{ matrix.toolchain }}, ${{ matrix.features }}
         run: |
           cargo +${{ matrix.toolchain }} build \
-              --release --all-features --all-targets
+              --release ${{ matrix.features }} --all-targets

-      - name: Test with ${{ matrix.toolchain }}
+      - name: Test with ${{ matrix.toolchain }}, ${{ matrix.features }}
         run: |
           if [[ "${{ matrix.toolchain }}" == 'nightly' ]]; then
               export RUSTDOCFLAGS='-Zsanitizer=address'
@@ -47,7 +48,7 @@ jobs:
           fi
           cargo +${{ matrix.toolchain }} test \
               --target=x86_64-unknown-linux-gnu \
-              --all-features
+              ${{ matrix.features }}

       - name: Lint with ${{ matrix.toolchain }}
         run: |

I've not tested it, but if I didn't make any mistake it should add a workflow job to build and run the (available) tests without the std feature.

SzymonKubica commented 4 months ago

Ok I think all of the missing / redundant blank lines should be gone now. Sorry about these, I will pay closer attention to the diff from now on so that you don't have to pick up on those. I agree that they shouldn't be there in the diff as those aren't any meaningful changes and they just clutter changes.

SzymonKubica commented 4 months ago

It could be interesting to add the following change as an additional commit to your PR:

diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
index 60eefc28d727..a11391c430ca 100644
--- a/.github/workflows/test.yaml
+++ b/.github/workflows/test.yaml
@@ -17,10 +17,11 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
+        toolchain: [stable, beta, nightly]
+        features: [--all-features]
         include:
-          - toolchain: stable
-          - toolchain: beta
           - toolchain: nightly
+            features: --no-default-features

     steps:
       - name: Checkout code
@@ -33,12 +34,12 @@ jobs:
           override: true
           components: clippy

-      - name: Build with ${{ matrix.toolchain }}
+      - name: Build with ${{ matrix.toolchain }}, ${{ matrix.features }}
         run: |
           cargo +${{ matrix.toolchain }} build \
-              --release --all-features --all-targets
+              --release ${{ matrix.features }} --all-targets

-      - name: Test with ${{ matrix.toolchain }}
+      - name: Test with ${{ matrix.toolchain }}, ${{ matrix.features }}
         run: |
           if [[ "${{ matrix.toolchain }}" == 'nightly' ]]; then
               export RUSTDOCFLAGS='-Zsanitizer=address'
@@ -47,7 +48,7 @@ jobs:
           fi
           cargo +${{ matrix.toolchain }} test \
               --target=x86_64-unknown-linux-gnu \
-              --all-features
+              ${{ matrix.features }}

       - name: Lint with ${{ matrix.toolchain }}
         run: |

I've not tested it, but if I didn't make any mistake it should add a workflow job to build and run the (available) tests without the std feature.

Thank you very much for proposing this change, It would have taken me a lot longer to figure out how the ci works and how to enable the no_std in the tests workflow. I realise that the last pipeline has failed but that was because I was missing the toolchain section in the include. Now I think it should work correctly, the test have failed because there was not toolchain specified in the last no_std test run

qmonnet commented 4 months ago

The tests pass correctly this time :tada:, but we get warnings about cargo-clippy. I'm not sure why, maybe using --no-default-features disables something clippy-related. I'll take a look later.

SzymonKubica commented 4 months ago

The tests pass correctly this time 🎉, but we get warnings about cargo-clippy. I'm not sure why, maybe using --no-default-features disables something clippy-related. I'll take a look later.

I think the reason this started producing warnings was that running cargo clippy implicitly adds a cargo-clippy feature that isn't included in the Cargo.toml. It looks like rbpf uses a legacy way of specifying clippy lint attributes (possible explanation here). Because of this, when running with --no-default-feature this cargo-clippy feature is missing and we get warnings.

I also looked through the test logs and got rid of all other warnings inside of misc.rs and rbpf_plugin.rs. All of them were mostly unused imports or functions that weren't used when running in no_std.

After these changes all builds were passing and tests didn't introduce any new warnings. However, I needed to squash the fix commits to make a single one that deals with all of the warnings. Could you please approve this workflow so that we can see that indeed everything got fixed? Thank you for your time, it's starting to get very exciting as we are getting closer having everything integrated :tada:

SzymonKubica commented 4 months ago

Ok @qmonnet I think all checks were successful and after running tests with --no-default-features there were no warnings emitted :tada: :rocket: . Thank you for helping me enable tests for the no_std feature. Do let me know if anything else requires attention in the PR and I will adjust it.

qmonnet commented 4 months ago

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

SzymonKubica commented 4 months ago

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.

SzymonKubica commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

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.

SzymonKubica commented 4 months ago

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.

SzymonKubica commented 4 months ago

Perfect, thank you for approving the PR :rocket: 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 commented 3 months ago

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

SzymonKubica commented 3 months ago

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

Thank you!

qmonnet / rbpf