Repository navigation
Conversation
SYS_READ returns the number of bytes not filled in the buffer. For a read that filled only part of the buffer, write_buffer_to_target returned the number of bytes read instead, so the target computed the wrong count from it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
paoValle
left a comment
paoValle
left a comment
There was a problem hiding this comment.
The direction is right: ARM semihosting SYS_READ returns the number of bytes not filled, so for a request of len bytes that read n the status must be len - n, and the old len => len as i32 arm returned the filled count instead. The pre-existing assert!(buf.len() <= self.len as usize) also keeps the subtraction from underflowing, which is the thing I would otherwise have flagged.
Three small things:
-
Nothing pins the new behaviour.
semihosting.rshas no test forwrite_buffer_to_target, and this is exactly the kind of value that regressed once already (inverted for partial reads, correct only for the full and empty cases). Pulling the arithmetic into a pure helper —fn unfilled_bytes(&self, filled: usize) -> i32— would let you test all four cases without aCore: 0 filled, partial, exactlyself.len, and afilled > self.lenpanic if you keep the assert. -
status as i32is applied toself.len, which comes from the target and is au32. A request for more thani32::MAXbytes now writes a negative status, which the target reads back as a huge positive value. This shape predates the PR (the0 => self.len as i32arm had it too), so it is not a regression, but since the formula now touches it directly ai32::try_from(..).unwrap_or(i32::MAX)with a one-line comment would close it. -
The doc comment still reads only "Writes the buffer to the target", while the function's other output — the status register write, derived from the unfilled count — is the thing this PR fixes. One sentence there would save the next reader the trip to the ARM spec.
|
LLM-generated review comment on an LLM-generated PR. What a world we live in 😔 |
ReadRequest::write_buffer_to_targetreturns the number of bytes read when a read fills only part of the buffer.SYS_READreturns the number of bytes not filled in the buffer (buffer_length - bytes_read), and targets derive the count from that; thesemihostingcrate, for example, keepslen - not_readbytes. Reading a 10-byte file with a 64-byte buffer therefore handed the target 54 bytes: the 10 from the file and 44 bytes the host never wrote. This hits the last read of most files, and short reads from TCP and UNIX stream handles.The fix returns
len - bytes_readfor every read. A full buffer still returns 0 and an empty read still returnslen.I have not compiled or run this locally (no Rust toolchain on this machine). I checked it against the spec and against OpenOCD's
semihosting_common.c, which also returnslen - result. There is no new test:write_buffer_to_targetneeds aCore, and the fake core inprobe/fake_probe.rsdoes not keep register writes, so a test could only repeat the subtraction.🤖 Generated with Claude Code