perf(auxflash): remove arithmetic panics in Cosmo auxflash #2296
Conversation
aapoalas
force-pushed
the
perf/auxflash-remove-panics
branch
from
0881390 to
4649a71
Compare
| let pos = inner_offset | ||
| - inner_chunk.header().total_len_in_bytes() as u64 | ||
| + core::mem::size_of::<tlvc::ChunkHeader>() as u64; | ||
| let pos = u32::try_from(inner_chunk.body_position()) |
There was a problem hiding this comment.
note: I was looking at the original code here and became convinced that this just wants the body_position from inner_chunk. There's still a chance I'm wrong, I didn't drag a comb through the code to really, truly convince myself of it but definitely the inner_offset is the position of the next chunk, and offsetting that by the total size - header size should give us the previous chunk's body position.
It seems prudent to just add an API to expose that location to tlvc; I have added that API to the PR that I have open in tlvc.
| self.active_slot.ok_or(AuxFlashError::NoActiveSlot)?; | ||
|
|
||
| let spare_slot = active_slot ^ 1; | ||
| const { |
There was a problem hiding this comment.
note: I've added these const-assertions here and there to try and help the compiler understand (locally) that checking active_slot < SLOT_COUNT also asserts that active_slot * SLOT_SIZE + SLOT_SIZE cannot overflow etc. Sometimes it works, but not every time.
| // in pairs as per above check, so flipping the 0th bit keeps slot | ||
| // validity. | ||
| unsafe { | ||
| assert_unchecked(active_slot < SLOT_COUNT); |
There was a problem hiding this comment.
thought: Having a type for the slot number would seem pretty useful.
| } | ||
| let data_size = SLOT_SIZE - reader.remaining() as usize; | ||
| // SAFETY: this cannot wrap, as our reader's max size is SLOT_SIZE. | ||
| let data_size = |
There was a problem hiding this comment.
note: Nope, the compiler couldn't figure this one out :(
|
|
||
| read_addr += amount; | ||
| write_addr += amount; | ||
| // SAFETY: these cannot overflow as amount is at most |
There was a problem hiding this comment.
note: Nope, compiler is unable to figure this one out for itself :(
| let amount = (data_len - mem_offset).min(buf.len()); | ||
| // SAFETY: amount takes us to data_len in at most buf sized | ||
| // chunks. The next offset is always at most data_len. | ||
| let next_mem_offset = unsafe { mem_offset.unchecked_add(amount) }; |
There was a problem hiding this comment.
note: Nope, not even this one :(
| // How much space is in the FIFO? | ||
| let fl = usize::from(sr.flevel().bits()); | ||
| let ffree = FIFO_SIZE - fl; | ||
| let ffree = FIFO_SIZE.saturating_sub(fl); |
There was a problem hiding this comment.
note: Easy panic path here to remove; saturating arithmetic is what we know this comes out to, so it's an easy choice. Equally well it would be fine to make this unchecked.
1 participant
No description provided.