Fix or allow all hard clippy errors. #11409

afck · 2020-01-26T17:19:26Z

This would make cargo clippy succeed, albeit with lots of warnings.

I think the write calls are an actual potential bug, since the returned number of written bytes is not handled. (I.e. it would just continue, even if the strings were only partially written.)

I'm very unsure about the alignment issues.

parity-cla-bot · 2020-01-26T17:19:29Z

It looks like @afck signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

afck · 2020-01-27T09:41:45Z

We could also #![allow(clippy::cast_ptr_alignment)] for the whole file in those three cases, although that would be even scarier. 😅

niklasad1 · 2020-01-27T09:42:38Z

ethash/src/cache.rs

@@ -185,6 +185,7 @@ fn make_memmapped_cache(path: &Path, num_nodes: usize, ident: &H256) -> io::Resu

 	let mut memmap = unsafe { MmapMut::map_mut(&file)? };

+	#[allow(clippy::cast_ptr_alignment)] // TODO: Why is this okay?


We should use ptr::read_unaligned instead but it will still be a clippy error see rust-lang/rust-clippy#2881

I'm still a bit confused about what's going on here. ~~But it really looks wrong to me:~~

initialize_memory internally creates a slice::from_raw_parts_mut of type &mut [Node].

~~MmapMut does not guarantee alignment, otherwise it wouldn't have an align_to_mut method.~~

from_raw_parts_mut explicitly requires the data to be aligned.

read_unaligned would copy the data: My impression is that most of this code was written as an optimization to avoid copying. If we are fine with copying, we could probably do away with most of the unsafe code here?

I thought mmap is page-aligned http://man7.org/linux/man-pages/man2/mmap.2.html, but maybe @Vurich could provide more input.

MmapMut does not guarantee alignment, otherwise it wouldn't have an align_to_mut method.

This is a method on a slice though (I guess it shows up in the docs, because MapMut implements Deref to a slice), which can be arbitrary aligned.

And I agree we should remove unsafe code as much as possible, we have some benchmarks, see e.g. #10861. But let's do it in a separate PR.

Ah, I see! Sorry! 🤦‍♂️
Then I guess I should just add a comment pointing to the mmap man page? (But what about platforms other than Linux?)

I thought mmap is page-aligned http://man7.org/linux/man-pages/man2/mmap.2.html, but maybe @Vurich could provide more input.

MmapMut does not guarantee alignment, otherwise it wouldn't have an align_to_mut method.

This is a method on a slice though (I guess it shows up in the docs, because MapMut implements Deref to a slice), which can be arbitrary aligned.

I have been summoned! The link you want is https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html - while on Linux it is defined to be page-aligned, mmap is a POSIX syscall and so on non-Linux POSIX environments it might fail. I'd add an alignment check (with error on fail) here, LLVM has alignment knowledge for libc functions for all architectures it supports so we can rely on it to remove the alignment check where appropriate. I'd be shocked if there are any real-world environments that will return unaligned pointers from mmap but better do the right thing now instead of dealing with a truly nasty bug further down the road.

I made the same mistake while writing this code, of using the Linux docs instead of the POSIX docs.

accounts/ethstore/src/secret_store.rs

niklasad1 · 2020-01-27T10:53:29Z

util/network-devp2p/src/node_table.rs

@@ -83,6 +83,7 @@ impl NodeEndpoint {
 		let address = match addr_bytes.len() {
 			4 => Ok(SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(addr_bytes[0], addr_bytes[1], addr_bytes[2], addr_bytes[3]), tcp_port))),
 			16 => unsafe {
+				#[allow(clippy::cast_ptr_alignment)] // TODO: Why is this okay?


well, I'm not super intimate with this piece of code why it is unsafe in the first place but I think it is because we don't want to copy the bytes to perform the conversion [u8; 16] -> [u16; 8]

We could do this in safe rust:

let mut addr = [0_u8; 16]; addr.copy_from_slice(&addr_bytes[0..16]); Ok(SocketAddr::V6(SocketAddrV6::new(Ipv6Addr::from(addr), tcp_port, 0, 0)))

The unsafe scope could be reduced because only slice::from_raw_parts is unsafe.

I'm up making it safe, but let's here what the others have to say //cc @ordian @dvdplm

AFAICT it is not used on any hot paths (on_ping, on_pong and on_neighbours). It is called often, sure, but not so often as to be a performance bottleneck I'd say. On the other hand the code today is, I'd say, safe.

I agree with @niklasad1, we should just replace it with the safe code unless it introduces a perf bottleneck.

ordian · 2020-01-27T13:25:25Z

ethash/src/cache.rs

@@ -265,6 +266,7 @@ fn read_from_path(path: &Path) -> io::Result<Vec<Node>> {
 	}

 	let out: Vec<Node> = unsafe {
+		#[allow(clippy::cast_ptr_alignment)] // TODO: Why is this okay?


hmm, this is a good question, I'm not sure myself about the alignment cc @Vurich

afck · 2020-03-10T14:05:44Z

Sorry for the delay! I added alignment checks and removed some unsafe code.

ordian · 2020-03-10T14:54:57Z

ethash/src/cache.rs

-	unsafe { initialize_memory(memmap.as_mut_ptr() as *mut Node, num_nodes, ident) };
+	unsafe {
+		let bytes = memmap.as_mut_ptr();
+		assert_eq!(bytes as usize % mem::align_of::<Node>(), 0, "Mmap memory not aligned.");


what's the rationale of panicking instead of returning an Error as in the code below?

is there any way to check this at compile-time?

mem::align_of is const fn

ordian · 2020-03-10T14:55:43Z

please merge with master, that should help with CI

util/EIP-152/src/avx2.rs

afck · 2020-03-11T11:00:51Z

I rebased on master, but I'm still getting errors on CI:

failed to run custom build command for `parity-snappy-sys v0.1.2`

and:

The process 'C:\Rust\.cargo\bin\cargo.exe' failed with exit code 101

This makes clippy succeed, albeit with lots of warnings.

Co-Authored-By: Niklas Adolfsson <[email protected]>

afck marked this pull request as ready for review January 27, 2020 09:40

niklasad1 reviewed Jan 27, 2020

View reviewed changes

accounts/ethstore/src/secret_store.rs Show resolved Hide resolved

niklasad1 reviewed Jan 27, 2020

View reviewed changes

ordian reviewed Jan 27, 2020

View reviewed changes

ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 28, 2020

afck force-pushed the afck-clippy-errs branch from 9530e9b to de688da Compare March 10, 2020 14:04

ordian reviewed Mar 10, 2020

View reviewed changes

afck force-pushed the afck-clippy-errs branch from de688da to 5928b08 Compare March 11, 2020 10:28

niklasad1 reviewed Mar 11, 2020

View reviewed changes

util/EIP-152/src/avx2.rs Show resolved Hide resolved

afck and others added 3 commits March 31, 2020 13:43

Fix or allow all hard clippy errors.

b543f31

This makes clippy succeed, albeit with lots of warnings.

Add alignment checks.

16ed10c

Add a comment to allow(clippy::cast_ptr_alignment).

d9a4c75

Co-Authored-By: Niklas Adolfsson <[email protected]>

afck force-pushed the afck-clippy-errs branch from 6b191ae to d9a4c75 Compare March 31, 2020 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix or allow all hard clippy errors. #11409

Fix or allow all hard clippy errors. #11409

afck commented Jan 26, 2020

parity-cla-bot commented Jan 26, 2020

afck commented Jan 27, 2020 •

edited

Loading

niklasad1 Jan 27, 2020 •

edited

Loading

afck Jan 27, 2020 •

edited

Loading

ordian Jan 27, 2020

ordian Jan 27, 2020 •

edited

Loading

afck Jan 27, 2020

eira-fransham Jan 28, 2020 •

edited

Loading

niklasad1 Jan 27, 2020

dvdplm Jan 27, 2020

ordian Jan 27, 2020

ordian Jan 27, 2020

afck commented Mar 10, 2020

ordian Mar 10, 2020

niklasad1 Mar 11, 2020

ordian commented Mar 10, 2020

afck commented Mar 11, 2020

		@@ -185,6 +185,7 @@ fn make_memmapped_cache(path: &Path, num_nodes: usize, ident: &H256) -> io::Resu

		let mut memmap = unsafe { MmapMut::map_mut(&file)? };

		#[allow(clippy::cast_ptr_alignment)] // TODO: Why is this okay?

Fix or allow all hard clippy errors. #11409

Are you sure you want to change the base?

Fix or allow all hard clippy errors. #11409

Conversation

afck commented Jan 26, 2020

parity-cla-bot commented Jan 26, 2020

afck commented Jan 27, 2020 • edited Loading

niklasad1 Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

afck Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Jan 28, 2020 • edited Loading

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

afck commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian commented Mar 10, 2020

afck commented Mar 11, 2020

afck commented Jan 27, 2020 •

edited

Loading

niklasad1 Jan 27, 2020 •

edited

Loading

afck Jan 27, 2020 •

edited

Loading

ordian Jan 27, 2020 •

edited

Loading

eira-fransham Jan 28, 2020 •

edited

Loading