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

Transmute big endian s6_addr and [u16; 8] #75085

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 3, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@tesuji tesuji marked this pull request as draft August 3, 2020 06:23
@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

Wow. Lots of tests are falling. I should reconsider this approach.

h as u8,
],
// SAFETY: Each element of `s6_addr16` is big edian.
s6_addr: unsafe { in6_union { s6_addr16: [a, b, c, d, e, f, g, h] }.s6_addr },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assumption here is wrong about arguments: (a, b, ...).
Those are not guaranteed to be Big Endian.

Copy link
Member

Choose a reason for hiding this comment

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

Right, the parameters are native-endian, and the output from segments is native too.

Maybe you could use [a.to_be_bytes(), b.to_be_bytes, ...] to get [[u8; 2]; 8] instead, and then cast that into s6_addr. But once we're dealing with byte swaps, your godbolt advantage may disappear, so I think this will only be a matter of code style.

Copy link
Member

Choose a reason for hiding this comment

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

Or [a.to_be(), b.to_be(), ...] to get a [u16; 8] directly that is big endian.

Copy link
Member

Choose a reason for hiding this comment

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

This actually does lead to better instructions: https://rust.godbolt.org/z/dv4Pje

@retep998
Copy link
Member

retep998 commented Aug 3, 2020

All members of the IN6_ADDR structure must be specified in network-byte-order (big-endian).

I'm pretty sure this means that if you're on a little endian system you have to byte swap your shorts before writing them to this structure. So you'd have to do .to_be() on each value.

@tesuji
Copy link
Contributor Author

tesuji commented Aug 3, 2020

Thanks both. I will resolve it later.
Do you have an interest to take a look at #75110 ?
In that PR, I use newtype to enforce endianness for Ipv4Addr. But beware that it is a draft one
and I'm figuring out how a lot of tests are falling.

@tesuji tesuji force-pushed the ip_union branch 2 times, most recently from a849a4a to 8eacf7d Compare August 5, 2020 08:45
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@tesuji tesuji force-pushed the ip_union branch 2 times, most recently from 44c7f27 to c7d37c5 Compare August 5, 2020 09:46
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Aug 8, 2020

Instead of the temporary union, have you tried transmute? (with #![feature(const_fn_transmute)])
It seems to codegen identically, so LLVM even deduplicates these: https://rust.godbolt.org/z/1Khbee
(Plus transmute already includes the size assertion.)

@tesuji
Copy link
Contributor Author

tesuji commented Aug 9, 2020

transmute already includes the size assertion.

Where is it defined? I can only find this:

fn codegen_transmute(&mut self, bx: &mut Bx, src: &mir::Operand<'tcx>, dst: mir::Place<'tcx>) {
if let Some(index) = dst.as_local() {
match self.locals[index] {
LocalRef::Place(place) => self.codegen_transmute_into(bx, src, place),
LocalRef::UnsizedPlace(_) => bug!("transmute must not involve unsized locals"),
LocalRef::Operand(None) => {
let dst_layout = bx.layout_of(self.monomorphized_place_ty(dst.as_ref()));
assert!(!dst_layout.ty.has_erasable_regions());
let place = PlaceRef::alloca(bx, dst_layout);
place.storage_live(bx);
self.codegen_transmute_into(bx, src, place);
let op = bx.load_operand(place);
place.storage_dead(bx);
self.locals[index] = LocalRef::Operand(Some(op));
self.debug_introduce_local(bx, index);
}
LocalRef::Operand(Some(op)) => {
assert!(op.layout.is_zst(), "assigning to initialized SSAtemp");
}
}
} else {
let dst = self.codegen_place(bx, dst.as_ref());
self.codegen_transmute_into(bx, src, dst);
}
}
fn codegen_transmute_into(
&mut self,
bx: &mut Bx,
src: &mir::Operand<'tcx>,
dst: PlaceRef<'tcx, Bx::Value>,
) {
let src = self.codegen_operand(bx, src);
let llty = bx.backend_type(src.layout);
let cast_ptr = bx.pointercast(dst.llval, bx.type_ptr_to(llty));
let align = src.layout.align.abi.min(dst.align);
src.val.store(bx, PlaceRef::new_sized_aligned(cast_ptr, src.layout, align));
}

@tesuji
Copy link
Contributor Author

tesuji commented Aug 9, 2020

Instead of the temporary union, have you tried transmute?

It would work. But we may lose all safety comments that documented in Ipv6Bytes.

@pickfire
Copy link
Contributor

pickfire commented Aug 9, 2020

It would work. But we may lose all safety comments that documented in Ipv6Bytes.

We could move those safety comments to unsafe transmute, the good thing is it will be shorter, the bad thing is hard to test and track the unsafe (probably).

Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Please update the PR title to "transmute".

library/std/src/net/ip.rs Outdated Show resolved Hide resolved
library/std/src/net/ip.rs Outdated Show resolved Hide resolved
@tesuji tesuji changed the title Use union to cast between big endian fields of s6_addr Transmute big endian s6_addr and [u16; 8] Aug 10, 2020
@cuviper
Copy link
Member

cuviper commented Aug 10, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 10, 2020

📌 Commit 0210fd3 has been approved by cuviper

@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 10, 2020
@bors
Copy link
Contributor

bors commented Aug 10, 2020

⌛ Testing commit 0210fd3 with merge 6b1e00131ee7682486cfc5f45ffb1966b89d4a81...

@bors
Copy link
Contributor

bors commented Aug 10, 2020

💔 Test failed - checks-azure

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

tesuji commented Aug 10, 2020

Hmm, macOS builds didn't start.

@cuviper
Copy link
Member

cuviper commented Aug 10, 2020

@bors retry rollup

@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 10, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#74744 (Update RELEASES.md for 1.46.0)
 - rust-lang#75085 (Transmute big endian `s6_addr` and `[u16; 8]`)
 - rust-lang#75226 (Miri: Renamed "undef" to "uninit")
 - rust-lang#75333 (polymorphize: constrain unevaluated const handling)
 - rust-lang#75338 (move stack size check to const_eval machine)
 - rust-lang#75347 (Rustdoc: Fix natural ordering to look at all numbers.)
 - rust-lang#75352 (Tweak conditions for E0026 and E0769)
 - rust-lang#75353 (Tiny cleanup, remove unnecessary `unwrap`)
 - rust-lang#75359 (unused_delims: trim expr)
 - rust-lang#75360 (Add sample fix for E0749)

Failed merges:

r? @ghost
@bors bors merged commit f26f201 into rust-lang:master Aug 11, 2020
@tesuji tesuji deleted the ip_union branch August 11, 2020 14:44
tmandry added a commit to tmandry/rust that referenced this pull request Sep 2, 2020
Make all methods of `std::net::Ipv6Addr` const

Make the following methods of `std::net::Ipv6Addr` unstable const under the `const_ipv6` feature:
- `segments`
- `is_unspecified`
- `is_loopback`
- `is_global` (unstable)
- `is_unique_local`
- `is_unicast_link_local_strict`
- `is_documentation`
- `multicast_scope`
- `is_multicast`
- `to_ipv4_mapped`
- `to_ipv4`

This would make all methods of `Ipv6Addr` const.

Changed the implementation of `is_unspecified` and `is_loopback` to use a `match` instead of `==`, all other methods did not require a change.

All these methods are dependent on `segments`, the current implementation of which requires unstable `const_fn_transmute` ([PR#75085](rust-lang#75085)).

Part of rust-lang#76205
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2020
Stabilize all stable methods of `Ipv4Addr`, `Ipv6Addr` and `IpAddr` as const

This PR stabilizes all currently stable methods of `Ipv4Addr`, `Ipv6Addr` and `IpAddr` as const.
Tracking issue: rust-lang#76205

`Ipv4Addr` (`const_ipv4`):
 - `octets`
 - `is_loopback`
 - `is_private`
 - `is_link_local`
 - `is_multicast`
 - `is_broadcast`
 - `is_docmentation`
 - `to_ipv6_compatible`
 - `to_ipv6_mapped`

`Ipv6Addr` (`const_ipv6`):
 - `segments`
 - `is_unspecified`
 - `is_loopback`
 - `is_multicast`
 - `to_ipv4`

`IpAddr` (`const_ip`):
 - `is_unspecified`
 - `is_loopback`
 - `is_multicast`

## Motivation
The ip methods seem like prime candidates to be made const: their behavior is defined by an external spec, and based solely on the byte contents of an address. These methods have been made unstable const in the beginning of September, after the necessary const integer arithmetic was stabilized.

There is currently a PR open (rust-lang#78802) to change the internal representation of `IpAddr{4,6}` from `libc` types to a byte array. This does not have any impact on the constness of the methods.

## Implementation
Most of the stabilizations are straightforward, with the exception of `Ipv6Addr::segments`, which uses the unstable feature `const_fn_transmute`. The code could be rewritten to equivalent stable code, but this leads to worse code generation (rust-lang#75085).
This is why `segments` gets marked with `#[rustc_allow_const_fn_unstable(const_fn_transmute)]`, like the already const-stable `Ipv6Addr::new`, the justification being that a const-stable alternative implementation exists rust-lang#76206 (comment).

## Future posibilities
This PR const-stabilizes all currently stable ip methods, however there are also a number of unstable methods under the `ip` feature (rust-lang#27709). These methods are already unstable const. There is a PR open (rust-lang#76098) to stabilize those methods, which could include const-stabilization. However, stabilizing those methods as const is dependent on `Ipv4Addr::octets` and `Ipv6Addr::segments` (covered by this PR).
@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
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.

6 participants