Skip to content

[test cases] igmp generation #23

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

Merged
merged 4 commits into from
Feb 19, 2025
Merged

[test cases] igmp generation #23

merged 4 commits into from
Feb 19, 2025

Conversation

zeeshanlakhani
Copy link
Contributor

@zeeshanlakhani zeeshanlakhani commented Feb 18, 2025

Test cases for IGMP v2/v3 generation.

@zeeshanlakhani
Copy link
Contributor Author

zeeshanlakhani commented Feb 18, 2025

@FelixMcFelix: This is currently in draft status, as I wanted to chat about some validation pieces around the multicast address range and a few other fields. We'll take on validation in #2, separately.

@zeeshanlakhani zeeshanlakhani marked this pull request as ready for review February 18, 2025 15:20
Copy link
Collaborator

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Zeeshan -- the IGMP tests look good. There are a few nits around the added methods for IP addresses. I think in general we should be trying to match constness, naming, and matched IP ranges to what is done in core::net?

I was thinking about this since our From/Into are zero-cost there, and realised that in core every check is done on self.octets() rather than reaching through the ref. If we do the same we generate identical code to just inlining the equivalent core method:

/// AA
#[no_mangle]
pub const fn is_documentation(ip: &Ipv4Addr) -> bool {
    matches!(ip.inner, [192, 0, 2, _] | [198, 51, 100, _] | [203, 0, 113, _])
}

/// AA
#[no_mangle]
pub const fn is_documentation2(ip: &Ipv4Addr) -> bool {
    let ip = *ip;
    matches!(ip.inner, [192, 0, 2, _] | [198, 51, 100, _] | [203, 0, 113, _])
}

/// B
#[no_mangle]
pub const fn is_documentation3(ip: &Ipv4Addr) -> bool {
    let ip = core::net::Ipv4Addr::new(ip.inner[0], ip.inner[1], ip.inner[2], ip.inner[3]);
    ip.is_documentation()
}
`cargo asm` output. `is_documentation2` and `is_documentation3` are identical.
kyle@Mac oxpacket % cargo asm -p ingot-types
    Finished `release` profile [optimized + debuginfo] target(s) in 0.01s

Try one of those by name or a sequence number
 0 "<&T as core::fmt::Display>::fmt" [14, 57]
 2 "<ingot_types::error::CRStr as core::fmt::Debug>::fmt" [52]
 3 "<ingot_types::error::CRStr as core::fmt::Display>::fmt" [52]
 4 "<ingot_types::error::CRStrError as core::fmt::Display>::fmt" [18]
 5 "<ingot_types::error::PacketParseError as core::fmt::Display>::fmt" [64]
 6 "<ingot_types::error::ParseError as core::fmt::Display>::fmt" [130]
 7 "ingot_types::error::CRStr::new_unchecked::panic_cold_explicit" [15]
 8 "is_documentation" [67]
 9 "is_documentation2" [32]
10 "is_documentation3" [32]
kyle@Mac oxpacket % cargo asm -p ingot-types 8
    Finished `release` profile [optimized + debuginfo] target(s) in 0.01s

	.globl	_is_documentation
	.p2align	2
_is_documentation:
Lfunc_begin7:
	.cfi_startproc
	ldrb w8, [x0]
	cmp w8, #203
	b.eq LBB7_7
	cmp w8, #198
	b.eq LBB7_5
	cmp w8, #192
	b.ne LBB7_8
	ldrb w8, [x0, #1]
	cbz w8, LBB7_10
	mov w0, #0
	ret
LBB7_5:
	ldrb w8, [x0, #1]
	cmp w8, #51
	b.ne LBB7_8
	ldrb w8, [x0, #2]
	cmp w8, #100
	cset w0, eq
	ret
LBB7_7:
	ldrb w8, [x0, #1]
	cbz w8, LBB7_9
LBB7_8:
	mov w0, #0
	ret
	ldrb w8, [x0, #2]
	cmp w8, #113
	cset w0, eq
	ret
	ldrb w8, [x0, #2]
	cmp w8, #2
	cset w0, eq
	ret
kyle@Mac oxpacket % cargo asm -p ingot-types 9
    Finished `release` profile [optimized + debuginfo] target(s) in 0.01s

	.globl	_is_documentation2
	.p2align	2
_is_documentation2:
Lfunc_begin8:
	.cfi_startproc
	ldrb w8, [x0]
	ldrb w9, [x0, #1]
	ldrb w10, [x0, #2]
	cmp w9, #0
	mov w11, #113
	ccmp w10, w11, #0, eq
	cset w11, eq
	cmp w9, #51
	mov w12, #100
	ccmp w10, w12, #0, eq
	cset w12, eq
	cmp w9, #0
	ccmp w10, #2, #0, eq
	cset w9, eq
	cmp w8, #192
	csel w9, wzr, w9, ne
	cmp w8, #198
	csel w9, w12, w9, eq
	cmp w8, #203
	csel w0, w11, w9, eq
	ret
kyle@Mac oxpacket % cargo asm -p ingot-types 10
    Finished `release` profile [optimized + debuginfo] target(s) in 0.01s

	.globl	_is_documentation3
	.p2align	2
_is_documentation3:
Lfunc_begin9:
	.cfi_startproc
	ldrb w8, [x0]
	ldrb w9, [x0, #1]
	ldrb w10, [x0, #2]
	cmp w9, #0
	mov w11, #113
	ccmp w10, w11, #0, eq
	cset w11, eq
	cmp w9, #51
	mov w12, #100
	ccmp w10, w12, #0, eq
	cset w12, eq
	cmp w9, #0
	ccmp w10, #2, #0, eq
	cset w9, eq
	cmp w8, #192
	csel w9, wzr, w9, ne
	cmp w8, #198
	csel w9, w12, w9, eq
	cmp w8, #203
	csel w0, w11, w9, eq
	ret

Long way of saying I'm not averse to us doing a const conversion and calling core's methods.

Some of these methods are also kinda hard to define without a prefix length, which feels like we're retreading a lot of the work that went into oxnet. It's a bit sad that the zerocopy traits aren't on the core/std IpAddr types for it all to just work together, but I guess if we want to go between it should be free.

Copy link
Collaborator

@FelixMcFelix FelixMcFelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits, but otherwise looks good. It's kind of a shame that from_octets isn't yet stable in core, or this would be neater still. Thanks!

@zeeshanlakhani zeeshanlakhani merged commit 8f5e3b8 into main Feb 19, 2025
5 checks passed
@zeeshanlakhani zeeshanlakhani deleted the zl/igmp-generation branch February 19, 2025 19:07
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.

2 participants