Eliminate some more `transmute()` #8879

niklasad1 · 2018-06-12T19:56:41Z

No description provided.

niklasad1 · 2018-06-12T20:15:43Z

util/network-devp2p/src/ip_utils.rs

-					addr[5],
-					addr[6],
-					addr[7])),
+					((addr[1] as u16) << 8) | (addr[0] as u16),


addr is actually a [u8; 16] so no need to transmute it but requires some shifting to merge two bytes into a word (two bytes)

Test: https://play.rust-lang.org/?gist=b26a380a7d08aa4b781508fc6d9f895e&version=stable&mode=debug

debris · 2018-06-13T07:56:17Z

util/network-devp2p/src/ip_utils.rs

@@ -236,16 +236,15 @@ mod getinterfaces {
 				let sa: *const sockaddr_in6 = sa as *const sockaddr_in6;
 				let sa = & unsafe { *sa };
 				let (addr, port) = (sa.sin6_addr.s6_addr, sa.sin6_port);


what is the type of addr here?

It is u32 but I saw that this was actually from std::net and it actually implements the from trait for our types so I changed it! Sorry for changing after reviewing byt it makes the code more readable IMO!

andresilva

Some grumbles.

andresilva · 2018-06-14T09:20:30Z

ethash/src/compute.rs

@@ -137,11 +137,11 @@ fn hash_compute(light: &Light, full_size: usize, header_hash: &H256, nonce: u64)
 		($n:expr, $value:expr) => {{
 			// We use explicit lifetimes to ensure that val's borrow is invalidated until the
 			// transmuted val dies.
-			unsafe fn make_const_array<'a, T, U>(val: &'a mut [T]) -> &'a mut [U; $n] {
+			unsafe fn make_const_array<'a, T, U>(val: &mut [T]) -> &mut [U; $n] {


Drop unsafe?

&mut *(val.as_mut_ptr() as *mut [U; $n]) is also unsafe because deref of raw pointer so I would like to keep the function as unsafe! Thoughts?

Yeah won't compile without unsafe ignore my comment.

looks like the explicit lifetimes have been dropped here. should be &'a mut [T] -> &'a mut [U; $n] right?

@rphmeier Explicit lifetimes here are not needed here, so I removed them but forgot a in the signature!

I can revert this if you want explicit lifetime annotations!

andresilva · 2018-06-14T09:20:59Z

ethash/src/compute.rs

@@ -268,10 +268,10 @@ fn hash_compute(light: &Light, full_size: usize, header_hash: &H256, nonce: u64)

 	let value: H256 = unsafe {


Drop unsafe?

Nevermind, keccak_256::unchecked is unsafe.

andresilva · 2018-06-14T09:25:47Z

util/network-devp2p/src/ip_utils.rs

-					((addr & 0x00FF_0000) >> 16) as u8,
-					((addr & 0xFF00_0000) >> 24) as u8)),
-					port)
+				(IpAddr::V4(Ipv4Addr::from(addr)), port)


Seems like we're encoding the octets in reverse order compared to std.

Ok, I will check but I assumed the tests actually tested this?!

Yes you are right, I will revert this!

* Make unsafe block smaller * Use different byte-order than `std`, read words as big endian instead of little endian!

* Use `from_be` to work both for big and little endian * Ipv6 addresses were incorrectly `transmuted`

niklasad1 · 2018-06-18T15:11:10Z

util/network-devp2p/src/ip_utils.rs

-					port)
+				let ip_addr = Ipv6Addr::from(addr);
+				debug_assert!(addr == ip_addr.octets());
+				(IpAddr::V6(ip_addr), port)


I'm pretty convinced that this was a bug because

[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1] // ::1

was parsed as [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0]

Also with u8, endianness "shouldn´t" matter

So, I suspect casting/transmute it to u16 [0x1 0x2] will be:

Little endian: -> [0x2 0x1]

Big endian: -> [0x1 0x2]

5chdn · 2018-06-22T09:15:38Z

Needs a 2nd review. cc @andresilva

andresilva

LGTM

* master: Add type for passwords. (#8920) deps: bump fs-swap (#8953) Eliminate some more `transmute()` (#8879) Restrict vault.json permssion to owner and using random suffix for temp vault.json file (#8932) print SS.self_public when starting SS node (#8949) scripts: minor improvements (#8930)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: deps: bump fs-swap (openethereum#8953) Eliminate some more `transmute()` (openethereum#8879) Restrict vault.json permssion to owner and using random suffix for temp vault.json file (openethereum#8932) print SS.self_public when starting SS node (openethereum#8949) scripts: minor improvements (openethereum#8930) rpc: cap gas limit of local calls (openethereum#8943) docs: update changelogs (openethereum#8931) ethcore: fix compilation when using slow-blocks or evm-debug features (openethereum#8936) fixed blooms dir creation (openethereum#8941) Update hardcoded headers (openethereum#8925)

niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 12, 2018

niklasad1 commented Jun 12, 2018

View reviewed changes

5chdn added this to the 1.12 milestone Jun 13, 2018

debris approved these changes Jun 13, 2018

View reviewed changes

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 13, 2018

niklasad1 force-pushed the na-remove-more-transmute branch from b2a2cf8 to c88c831 Compare June 13, 2018 09:02

andresilva suggested changes Jun 14, 2018

View reviewed changes

andresilva added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jun 14, 2018

niklasad1 force-pushed the na-remove-more-transmute branch from c88c831 to 7090737 Compare June 14, 2018 13:45

niklasad1 added 2 commits June 18, 2018 16:00

eliminate some more transmute()

b7c06c1

Address review comments

884ed02

* Make unsafe block smaller * Use different byte-order than `std`, read words as big endian instead of little endian!

niklasad1 force-pushed the na-remove-more-transmute branch from 7090737 to 2199be7 Compare June 18, 2018 14:01

Fix IpAddresses nits

4a3f7d3

* Use `from_be` to work both for big and little endian * Ipv6 addresses were incorrectly `transmuted`

niklasad1 force-pushed the na-remove-more-transmute branch from 2199be7 to 4a3f7d3 Compare June 18, 2018 14:23

remove needless lifetime annotation

cd7d47a

niklasad1 commented Jun 18, 2018

View reviewed changes

openethereum deleted a comment from rphmeier Jun 18, 2018

andresilva approved these changes Jun 22, 2018

View reviewed changes

andresilva added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jun 22, 2018

andresilva merged commit 0cd1de7 into master Jun 22, 2018

5chdn deleted the na-remove-more-transmute branch June 22, 2018 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate some more `transmute()` #8879

Eliminate some more `transmute()` #8879

niklasad1 commented Jun 12, 2018

niklasad1 Jun 12, 2018

niklasad1 Jun 14, 2018

debris Jun 13, 2018

niklasad1 Jun 13, 2018 •

edited

Loading

andresilva left a comment

andresilva Jun 14, 2018

niklasad1 Jun 14, 2018

andresilva Jun 14, 2018

rphmeier Jun 18, 2018

niklasad1 Jun 18, 2018

andresilva Jun 14, 2018

andresilva Jun 14, 2018

andresilva Jun 14, 2018

niklasad1 Jun 14, 2018

niklasad1 Jun 14, 2018

niklasad1 Jun 18, 2018 •

edited

Loading

5chdn commented Jun 22, 2018

andresilva left a comment

		@@ -268,10 +268,10 @@ fn hash_compute(light: &Light, full_size: usize, header_hash: &H256, nonce: u64)

		let value: H256 = unsafe {

Eliminate some more transmute() #8879

Eliminate some more transmute() #8879

Conversation

niklasad1 commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

5chdn commented Jun 22, 2018

andresilva left a comment

Choose a reason for hiding this comment

Eliminate some more `transmute()` #8879

Eliminate some more `transmute()` #8879

niklasad1 Jun 13, 2018 •

edited

Loading

niklasad1 Jun 18, 2018 •

edited

Loading