Fix clippy warnings #80
Conversation
| let obj_opts = libbpf_sys::bpf_object_open_opts { | ||
| sz: std::mem::size_of::<libbpf_sys::bpf_object_open_opts>() as libbpf_sys::size_t, | ||
| object_name: cname.as_ptr(), | ||
| ..Default::default() |
There was a problem hiding this comment.
does this ensure that all the bytes are zeroed out, even padding?
There was a problem hiding this comment.
I don't think so.
Example: https://godbolt.org/z/r75T13Pcd
Rust generates code to call ::default() which is implemented by https://docs.rs/libbpf-sys/0.3.0-1/src/libbpf_sys/bindings.rs.html#2987-2991 and the implementation zeros out the struct using https://doc.rust-lang.org/stable/std/mem/fn.zeroed.html . From reading the docs there and looking at rust-lang/rust#70230 , I don't believe bindgen is generating the right code here. It should probably generate a memset.
Also note that this PR doesn't change the old behavior. Functionally it should be the same as before.
There was a problem hiding this comment.
There's definitely padding: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=b7342c8fe84d6bb1e01333e4e07f7a79 (48 vs 38). I think this should be a bindgen bug
There was a problem hiding this comment.
@danobi I think I'm following along, though I'm still p new to systems programming.
It sounds like the padding is bad because we want libbpf-rs to be as conservative as possible when it comes to memory, yeah? Also, It seems like rust-lang/rust#70230 is going to take some time to resolve, and when it does it'll be in the nightly release at first.
I can revert these changes around calling default() and add a comment so that clippy ignores em. That work?
There was a problem hiding this comment.
Just pushed up the revert - let me know if that's not the right path, and I can undo em
There was a problem hiding this comment.
I think your original changes were good. I think it's a bug with bindgen (a tool used to generate rust bindings to c/c++ libraries). I'm working on fixing the bindgen bug in rust-lang/rust-bindgen#2051
Even then, the non-zero padding is currently a theoretical issue. We haven't seen issues with it yet -- not to say it won't happen in the future (b/c bindgen is invoking undefined behavior)
|
Can you rebase and also remove the whitespace changes again? |
shaqq
force-pushed
the
fix-clippy-wqrnings
branch
from
ac0517a
to
350e50a
Compare
| @@ -165,10 +165,10 @@ impl<'a> ObjectSkeletonConfigBuilder<'a> { | |||
| // Holds `CString`s alive so pointers to them stay valid | |||
| let mut string_pool = Vec::new(); | |||
|
|
|||
| // NB: use default() to zero out struct | |||
There was a problem hiding this comment.
@danobi I also got rid of this, since it appears to be a comment about zeroing out the bytes, but it sounds like we'll fix it with the bind-gen bugfix
|
Thanks! |
@danobi I took a quick stab at getting rid of all warnings.
There were a bunch of errors in
examples/runqslower, but it didn't quite make sense to add Clippy ignore's in there, since people are likely going to copy/paste from that code. Let me know if I got that wrong.Otherwise, this is pretty much my first Rust code ever, so let me know what I should fix!