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

Replace uses of mem::transmute with safe methods #1983

Closed
wants to merge 1 commit into from

Conversation

djkoloski
Copy link
Contributor

These transmutes are unnecessary and technically incorrect since std doesn't guarantee that the byte order of their network address types matches the expected byte order of libc.

Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

bors r+

Thanks!

bors bot added a commit that referenced this pull request Jan 27, 2023
1983: Replace uses of `mem::transmute` with safe methods r=rtzoeller a=djkoloski

These transmutes are unnecessary and technically incorrect since std doesn't guarantee that the byte order of their network address types matches the expected byte order of libc.

Co-authored-by: David Koloski <djkoloski@gmail.com>
@rtzoeller
Copy link
Collaborator

bors r-

Realized that @asomers had made part of this change intentionally here. Reopening for his input.

@bors
Copy link
Contributor

bors bot commented Jan 27, 2023

Canceled.

@asomers
Copy link
Member

asomers commented Jan 27, 2023

I made that change partly for performance reasons, in order to make it truly zero-cost. But the main motivation was to make ipv4addr_to_libc a const fn. That wasn't originally possible using Ipv4Addr::octets prior to 1.50.0. And while djkoloski is correct that nothing technically requires std's Ip4Addr to be a simple newtype around libc::in_addr, that's by far the easiest way to do it. I checked that it was true at the time and added the static assertion for size, but I probably should've added a unit test too. The best option would be if there were some kind of Ip4Addr::Into<libc::in_addr> function, but I don't think std will want to add such a thing.

@djkoloski
Copy link
Contributor Author

Is there actually a performance benefit from using mem::transmute over the safe alternative? I tried the two alternatives out in compiler explorer.

As expected, the unsound approach is better under -C opt-level=0:

core::net::ip_addr::Ipv4Addr::octets:
  sub rsp, 4
  mov eax, dword ptr [rdi]
  mov dword ptr [rsp], eax
  mov eax, dword ptr [rsp]
  add rsp, 4
  ret

core::num::<impl u32>::from_ne_bytes:
  sub rsp, 12
  mov dword ptr [rsp + 4], edi
  mov eax, dword ptr [rsp + 4]
  mov dword ptr [rsp], eax
  mov eax, dword ptr [rsp]
  mov dword ptr [rsp + 8], eax
  mov eax, dword ptr [rsp + 8]
  add rsp, 12
  ret

example::ipv4addr_to_libc_1:
  sub rsp, 16
  mov dword ptr [rsp + 4], edi
  mov eax, dword ptr [rsp + 4]
  mov dword ptr [rsp], eax
  mov eax, dword ptr [rsp]
  mov dword ptr [rsp + 8], eax
  mov eax, dword ptr [rsp + 8]
  add rsp, 16
  ret

example::ipv4addr_to_libc_2:
  sub rsp, 24
  mov dword ptr [rsp + 4], edi
  mov eax, dword ptr [rsp + 4]
  mov dword ptr [rsp], eax
  mov rdi, rsp
  call core::net::ip_addr::Ipv4Addr::octets
  mov dword ptr [rsp + 20], eax
  mov eax, dword ptr [rsp + 20]
  mov dword ptr [rsp + 16], eax
  mov edi, dword ptr [rsp + 16]
  call core::num::<impl u32>::from_ne_bytes
  mov dword ptr [rsp + 8], eax
  mov eax, dword ptr [rsp + 8]
  add rsp, 24
  ret

But that advantage goes away as soon as any optimizations are enabled. -C opt-level=1 generates identical assembly for both implementations:

example::ipv4addr_to_libc_1:
  mov eax, edi
  ret

example::ipv4addr_to_libc_2:
  mov eax, edi
  ret

This holds for ARM assembly (--target=aarch64-unknown-linux-gnu1):

example::ipv4addr_to_libc_1:
  ret

example::ipv4addr_to_libc_2:
  ret

Notably, -C opt-level=2 and above don't even generate a function for the safe implementation but does for the unsafe one. It does appear to still inline both.

@SteveLauC
Copy link
Member

This PR has been replaced by #2061, so I will close it.

Sorry about this @djkoloski and thanks for your great work!

@SteveLauC SteveLauC closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants