aya, aya-obj: Implement ENUM64 fixups #725
Conversation
✅ Deploy Preview for aya-rs-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mergify
bot
added
aya
dave-tucker
force-pushed
the
enum64
branch
2 times, most recently
from
a3cc489
to
6b27f38
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @dave-tucker)
aya-obj/src/btf/btf.rs line 486 at r1 (raw file):
// ENUM64 placeholder type needs to be added before // we take ownership of self.types to ensure that // the offsets in the BtfHeader are correct
period, odd wrap width
aya-obj/src/btf/btf.rs line 487 at r1 (raw file):
// we take ownership of self.types to ensure that // the offsets in the BtfHeader are correct let mut enum64_placeholder_id = None;
this looks like bool::then
aya-obj/src/btf/btf.rs line 490 at r1 (raw file):
if self .types() .any(|t| t.kind() == BtfKind::Enum64 && !features.btf_enum64)
can we hoist features.btf_enum64 out of the iteration?
aya-obj/src/btf/btf.rs line 695 at r1 (raw file):
} BtfType::Enum(ty) if !features.btf_enum64 && ty.is_signed() => { debug!("{}: signed ENUMs not supported. Marking as unsigned", kind);
do we want to link to libbpf?
aya-obj/src/btf/btf.rs line 696 at r1 (raw file):
BtfType::Enum(ty) if !features.btf_enum64 && ty.is_signed() => { debug!("{}: signed ENUMs not supported. Marking as unsigned", kind); let mut ty = ty.clone();
how come we need to clone? could we write let t = &mut types.types[i] and just mutate it directly?
aya-obj/src/btf/btf.rs line 1751 at r1 (raw file):
assert_matches!(btf.type_by_id(enum_type_id).unwrap(), BtfType::Enum(fixed) => { assert!(!fixed.is_signed()); assert_eq!(fixed.variants.len(), 3);
can we use assert_matches here instead of separately asserting length and content?
aya-obj/src/btf/btf.rs line 1757 at r1 (raw file):
}); // Ensure we can convert to bytes and back again
periods in comments please.
aya-obj/src/btf/btf.rs line 1786 at r1 (raw file):
assert_matches!(btf.type_by_id(enum_type_id).unwrap(), BtfType::Union(fixed) => { assert_eq!(fixed.members.len(), 3);
suggest assert_matches here too, with a pattern of [_, _, _] so that it's obvious how to extent to check the values
aya-obj/src/btf/btf.rs line 1789 at r1 (raw file):
}); // Ensure we can convert to bytes and back again
ditto
aya-obj/src/btf/types.rs line 398 at r1 (raw file):
impl BtfEnum { pub fn new(name_offset: u32, value: u32) -> Self { BtfEnum { name_offset, value }
nit: s/BtfEnum/Self/
I did that for existing code in #726
aya-obj/src/btf/types.rs line 402 at r1 (raw file):
pub(crate) fn to_bytes(&self) -> Vec<u8> { let mut buf = vec![];
i think you can avoid multiple allocations here
let Self { name_offset, value } = self;
[bytes_of::<u32>(name_offset), bytes_of::<u32>(value)].into_iter().flatten().collect()
done for existing code in #726
aya-obj/src/btf/types.rs line 533 at r1 (raw file):
name_offset, info, size: 8, // docs say size is 1/2/4/8 but we'll assume 8
link to docs?
|
@dave-tucker, this pull request is now in conflict and requires a rebase. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dave-tucker)
aya-obj/src/btf/btf.rs line 487 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this looks like bool::then
let enum64_placeholder_id =
(!features.btf_enum64 && self.types().any(|t| t.kind() == BtfKind::Enum64)).then(|| {
self.add_type(BtfType::Int(Int::new(
self.add_string("enum64_placeholder"),
1,
IntEncoding::None,
0,
)))
});aya-obj/src/btf/btf.rs line 696 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
how come we need to clone? could we write
let t = &mut types.types[i]and just mutate it directly?
rather than do it here, why not do it where the existing let t = &types.types[i] is?
aya-obj/src/btf/btf.rs line 693 at r3 (raw file):
types.types[i] = int_type; } // Sanitize TYPE_TAG
periods on all of these?
aya-obj/src/btf/btf.rs line 1808 at r3 (raw file):
assert_matches!(fixed.members[..], [ BtfMember { name_offset: name_offset1, btf_type: btf_type1, offset:0 }, BtfMember { name_offset: name_offset2, btf_type:btf_type2, offset:0 },
a few missing spaces after colons
aya-obj/src/btf/types.rs line 664 at r3 (raw file):
pub(crate) fn new(name_offset: u32, size: u32, members: Vec<BtfMember>) -> Self { let info = (BtfKind::Union as u32) << 24; Union {
Self
This should get the aya integration tests passing, which are not yet able to run on older kernels. See aya-rs/aya#725 and aya-rs/aya#638.
|
Parking this one until #731 lands to handle the comment re: mutate in place |
dave-tucker
force-pushed
the
enum64
branch
2 times, most recently
from
692a9b8
to
7fa1b43
Compare
There was a problem hiding this comment.
Dismissed @tamird from a discussion.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @tamird)
aya-obj/src/btf/btf.rs line 486 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
period, odd wrap width
Done.
aya-obj/src/btf/btf.rs line 487 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
let enum64_placeholder_id = (!features.btf_enum64 && self.types().any(|t| t.kind() == BtfKind::Enum64)).then(|| { self.add_type(BtfType::Int(Int::new( self.add_string("enum64_placeholder"), 1, IntEncoding::None, 0, ))) });
Done.
aya-obj/src/btf/btf.rs line 490 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we hoist features.btf_enum64 out of the iteration?
Done.
aya-obj/src/btf/btf.rs line 695 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
do we want to link to libbpf?
Done at the top of the function since it applies to the function as a whole.
aya-obj/src/btf/btf.rs line 696 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
rather than do it here, why not do it where the existing
let t = &types.types[i]is?
Done.
aya-obj/src/btf/btf.rs line 1751 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
can we use assert_matches here instead of separately asserting length and content?
Done.
aya-obj/src/btf/btf.rs line 1757 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
periods in comments please.
Done.
aya-obj/src/btf/btf.rs line 1786 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
suggest assert_matches here too, with a pattern of
[_, _, _]so that it's obvious how to extent to check the values
Done.
aya-obj/src/btf/btf.rs line 1789 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
ditto
Done.
aya-obj/src/btf/btf.rs line 693 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
periods on all of these?
Done.
aya-obj/src/btf/btf.rs line 1808 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
a few missing spaces after colons
Done.
aya-obj/src/btf/types.rs line 402 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
i think you can avoid multiple allocations here
let Self { name_offset, value } = self; [bytes_of::<u32>(name_offset), bytes_of::<u32>(value)].into_iter().flatten().collect()done for existing code in #726
Done.
aya-obj/src/btf/types.rs line 533 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
link to docs?
Done.
aya-obj/src/btf/types.rs line 664 at r3 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
Self
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dave-tucker)
aya-obj/src/btf/btf.rs line 510 at r4 (raw file):
let kind = t.kind(); match t { // Fixup PTR for Rust.
blank comment lines between paragraphs here and everywhere please
aya-obj/src/btf/btf.rs line 568 at r4 (raw file):
} // There are some cases when the compiler does indeed populate the size
period
aya-obj/src/btf/btf.rs line 700 at r4 (raw file):
BtfType::Enum(ty) if !features.btf_enum64 && ty.is_signed() => { debug!("{}: signed ENUMs not supported. Marking as unsigned", kind); if let BtfType::Enum(t) = &mut types.types[i] {
can't this be ty.set_signed(false);?
aya-obj/src/btf/types.rs line 556 at r4 (raw file):
info, // According to the documentation: // https://www.kernel.org/doc/html/next/bpf/btf.html
blank line before and after this one
This commit adds: - A probe to see if the ENUM64 feature is supported - Fixups for the use of signed enums, or enum64 types on systems where enum64 is not supported Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dave-tucker)
This commit adds:
This change is