process_unix: prefer i32::*_be_bytes over manually shifting bytes #74271
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Unfortunately, I am somewhat confused by the PR :/
I am just very unfamiliar with that code. Could you just link me to some resources that explain the unix basics required for understanding this code? I mean, I wouldn't have to fully understand it if it weren't for that fact that, to me, it looks like your change is changing behavior.
| let errno = errno.to_be_bytes(); | ||
| let bytes = [ | ||
| (errno >> 24) as u8, | ||
| (errno >> 16) as u8, | ||
| (errno >> 8) as u8, | ||
| (errno >> 0) as u8, | ||
| errno[0], | ||
| errno[1], | ||
| errno[2], | ||
| errno[3], |
There was a problem hiding this comment.
Uhm... with this code, the errno is always encoded in big endian into the array. But before, it was always swapped -- even on big endian devices -- right? And I don't see any #[cfg(target_endianess = ...)] attributes anywhere. So your PR changes behavior, right? Which means either your change or the previous code is buggy? Is this code tested on big endian CPUs?
I unfortunately don't know a lot about these unix system APIs. Could you maybe link me some resource which specifies that this has to be big endian? Or is this just used internally? Like... the data is used below. But if it's just internally, why swap any bytes at all? Why not keep it in native endianess?
| fn combine(arr: &[u8]) -> i32 { | ||
| let a = arr[0] as u32; | ||
| let b = arr[1] as u32; | ||
| let c = arr[2] as u32; | ||
| let d = arr[3] as u32; | ||
|
|
||
| ((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32 | ||
| fn combine(arr: [u8; 4]) -> i32 { | ||
| i32::from_be_bytes(arr) |
There was a problem hiding this comment.
Same as above: before it always swapped, now it only swaps on little endian system.
There was a problem hiding this comment.
The generated asms are the same: https://rust.godbolt.org/z/ezbn7K .
IIUC, the old code is loading Big Endian bytes from arr.
arr[0] is most significant value, arr[3] is the least significant one.
There was a problem hiding this comment.
Thanks for your links, but I do know what endianness is 😄
I was rather asking for docs on why a specific endianness is required in this particular case/code (dealing with unix APIs).
However, I have been bamboozled by the code! I misunderstood the original code and therefore was very confused! Now everything makes sense again and I like that change!
|
@bors r+ rollup=iffy |
|
📌 Commit 879afd5 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 5, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
| assert!( | ||
| combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4..8]), | ||
| combine(CLOEXEC_MSG_FOOTER) == combine(footer.try_into().unwrap()), |
There was a problem hiding this comment.
Actually, why we not just compare 2 slice instead?
There was a problem hiding this comment.
Oops, that would be better indeed.
…arth Rollup of 11 pull requests Successful merges: - rust-lang#73759 (Add missing Stdin and StdinLock examples) - rust-lang#74211 (Structured suggestion when not using struct pattern) - rust-lang#74228 (Provide structured suggestion on unsized fields and fn params) - rust-lang#74252 (Don't allow `DESTDIR` to influence LLVM builds) - rust-lang#74263 (Slight reorganization of sys/(fast_)thread_local) - rust-lang#74271 (process_unix: prefer i32::*_be_bytes over manually shifting bytes) - rust-lang#74272 (pprust: support multiline comments within lines) - rust-lang#74332 (Update cargo) - rust-lang#74334 (bootstrap: Improve wording on docs for `verbose-tests`) - rust-lang#74336 (typeck: use `item_name` in cross-crate packed diag) - rust-lang#74340 (lint: use `transparent_newtype_field` to avoid ICE) Failed merges: r? @ghost
This PR makes it more clear about the intend of the code.