Skip to content
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

Use u32::from_ne_bytes to fix a FIXME and add comment about that #75086

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 3, 2020

u32::from_ne_bytes has been const stable since 1.44.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

Ooops, Scott seems to be busy. So r? @oli-obk as reviewer of #58392

@rust-highfive rust-highfive assigned oli-obk and unassigned scottmcm Aug 3, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

@bors r+ rollup

awesome!

@bors
Copy link
Contributor

bors commented Aug 3, 2020

📌 Commit edcb4058be815b5ab061f8efb1d235099de633fd has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

Eh CI is falling. I don't know if it is spurious or not.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

thread 'main' panicked at 'called Result::unwrap() on an Err value: Os { code: 99, kind: AddrNotAvailable, message: "Cannot assign requested address" }', /checkout/src/test/ui/fds-are-cloexec.rs:32:49

Sounds related at least, did these pass for you locally?

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 3, 2020
),
},
}
Ipv4Addr { inner: c::in_addr { s_addr: u32::from_be_bytes([a, b, c, d]) } }
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the fixme was wrong? I can never get be/le order right...

Copy link
Contributor

Choose a reason for hiding this comment

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

Jup, it's wrong: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=2920668bc321a75501f2bc033ed9f893

I guess we need from_le_bytes here. Makes sense since the function says the bytes already have to be in the right order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Will run the test locally first and update the PR later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think about it again. Do we guarantee that c::in_addr in big endian?
I get the feelings that we are missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect u32::from_ne_bytes([a, b, c, d]), since we want to store those as abcd (an internet address is stored in big-endian byte ordering). This would be equivalent to the old implementation. The u32::from_le_bytes([a, b, c, d]) swaps the order on big-endian targets to dcba.

Copy link
Contributor Author

@tesuji tesuji Aug 3, 2020

Choose a reason for hiding this comment

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

Those endiannesses are confusing in practice. Hope we have an way to enforce this in type system.

Copy link
Contributor Author

@tesuji tesuji Aug 3, 2020

Choose a reason for hiding this comment

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

Yeah, judge by assembly, I think ne is the correct answer: https://rust.godbolt.org/z/e5ahnK

Copy link
Member

Choose a reason for hiding this comment

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

Very likely that I was wrong in the TODO -- I think I was wrong in #57740 (comment) too

@tesuji tesuji changed the title Use u32::from_be_bytes to fix a FIXME Use u32::from_le_bytes to fix a FIXME Aug 3, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

Done.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2020

📌 Commit aca0258 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 3, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

pub fn octets(&self) -> [u8; 4] {
// This returns the order we want because s_addr is stored in big-endian.
self.inner.s_addr.to_ne_bytes()
}

But we are taking Little Endian in this PR.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

isn't ne and le the same thing?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

(should probably use ne for clarity then though)

@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

isn't ne and le the same thing?

ne is native byte order, could be le and be.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 3, 2020

oh, I thought it was network endian...

In that case the current use of le is the right one, as we manually created a le u32 and then invoked to_be on it, this was independent of the host platform.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

I mean, the tests pass so the current conversion is right.
But I am worry about we have no guarantee about endianness of c::in_addr.
But many systems assume that in_addr and in6_addr in big endian.

This PR could be merged but I think we need a issue to validate all assignments
and conversions to in_addr are valid.

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 3, 2020
Use u32::from_le_bytes to fix a FIXME

`u32::from_be_bytes` has been const stable since 1.44.
),
},
}
Ipv4Addr { inner: c::in_addr { s_addr: u32::from_le_bytes([a, b, c, d]) } }
Copy link
Member

Choose a reason for hiding this comment

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

All of these from_*_bytes return a native-endian u32. Right now you're presenting the bytes in BE order but calling it LE -- that will sort of work on LE targets, but will get swapped on BE targets to the wrong order.

I think it should really use from_be_bytes, and then you still need u32::to_be around it.

@cuviper
Copy link
Member

cuviper commented Aug 3, 2020

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 3, 2020
@cuviper
Copy link
Member

cuviper commented Aug 3, 2020

It should be u32::from_ne_bytes in new

Actually yes, u32::from_ne_bytes([a, b, c, d]) should do the right thing everywhere, giving a big-endian u32 regardless of the target endian. But that deserves a comment if we go this way, that we're intentionally hacking the order.

@adamreichold
Copy link
Contributor

adamreichold commented Aug 3, 2020

I think it should really use from_be_bytes, and then you still need u32::to_be around it.

Yes, you are right. The original code always ends with d at the lowest valued and a the highest valued byte, i.e. [d,c,d,a] on little endian and [a,b,c,d] on big endian machines.

Actually yes, u32::from_ne_bytes([a, b, c, d]) should do the right thing everywhere, giving a big-endian u32 regardless of the target endian. But that deserves a comment if we go this way, that we're intentionally hacking the order.

That sounds correct to me as well as it is the result of u32::from_be_bytes([a,b,c,d]).to_be() as it either swaps zero times or twice. But maybe that should still be written out and the compiler be assumed to optimize the swaps away? (At least it looks like it does: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=63e7fef5469483cfe3d52191473482e6)

@wwylele
Copy link
Contributor

wwylele commented Aug 3, 2020

Thanks for coming but to_ne_bytes returns native bytes, not network bytes like to_be_bytes.

I said "On a big-endian machine:", then ne is be

@cuviper
Copy link
Member

cuviper commented Aug 3, 2020

Yeah, it ultimately looks identical either way here: https://rust.godbolt.org/z/o4M1zz
You can also try that with --target s390x-unknown-linux-gnu for big-endian, then LLVM even merges the functions.

@scottmcm
Copy link
Member

scottmcm commented Aug 3, 2020

Hmm, this is basically the inverse of to_octets, right? That method is

    pub fn octets(&self) -> [u8; 4] {
        // This returns the order we want because s_addr is stored in big-endian.
        self.inner.s_addr.to_ne_bytes()
    }

So it seem like the right implementation here might just be

    pub const fn new(a: u8, b: u8, c: u8, d: u8) -> Ipv4Addr {
        // s_addr is stored in big-endian on all machines, so the array is provided in big-endian and
        // the native endian conversion method is used so that it's never swapped.
        Ipv4Addr { inner: c::in_addr { s_addr: u32::from_ne_bytes([a, b, c, d]) } }
    }

@cuviper
Copy link
Member

cuviper commented Aug 3, 2020

@scottmcm Yes -- and I think your comment would nicely explain what's going on there.

Co-authored-by: Weiyi Wang <wwylele@gmail.com>
Co-authored-by: Adam Reichold <adam.reichold@t-online.de>
Co-authored-by: Josh Stone <cuviper@gmail.com>
Co-authored-by: Scott McMurray <scottmcm@users.noreply.github.com>
Co-authored-by: tmiasko <tomasz.miasko@gmail.com>
@tesuji tesuji changed the title Use u32::from_le_bytes to fix a FIXME Use u32::from_ne_bytes to fix a FIXME and add comment about that Aug 5, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 5, 2020

Thanks all. I recommitted and listed all of you as the commit author.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2020
Note about endianness of returned value of {integer}::from_be_bytes and friends

[`u32::from_be`](https://doc.rust-lang.org/nightly/src/core/num/mod.rs.html#2883-2892) documents about endianness of returned value.

I was confused by endianness of `from_be_bytes` in rust-lang#75086 .
@tesuji
Copy link
Contributor Author

tesuji commented Aug 6, 2020

There is another interfere PR #75110 but I want this PR to land first because
there are changes that that PR will not be accepted.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 6, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 6, 2020

📌 Commit 30a1455 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2020
@bors
Copy link
Contributor

bors commented Aug 6, 2020

⌛ Testing commit 30a1455 with merge c15bae5...

@bors
Copy link
Contributor

bors commented Aug 6, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing c15bae5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 6, 2020
@bors bors merged commit c15bae5 into rust-lang:master Aug 6, 2020
@bors bors mentioned this pull request Aug 6, 2020
@tesuji tesuji deleted the u32const branch August 6, 2020 16:42
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 7, 2020
Note about endianness of returned value of {integer}::from_be_bytes and friends

[`u32::from_be`](https://doc.rust-lang.org/nightly/src/core/num/mod.rs.html#2883-2892) documents about endianness of returned value.

I was confused by endianness of `from_be_bytes` in rust-lang#75086 .
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants