Details

Reviewers

deadalnix
markblundeberg

Group Reviewers

Restricted Project

Commits

rSTAGINGd7be9aa41479: Fix -Wcast-align in crypto_hash.cpp
rABCd7be9aa41479: Fix -Wcast-align in crypto_hash.cpp

Summary

Avoid the C-style cast to get rid of the warning.

This does not change the behavior.

Test Plan

ninja bitcoin-bench

Check the warning disappeared.

./src/bench/bitcoin-bench -filter=SipHash_32b

Compare the benchmark result with master and check there is no
performance penalty.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

fix_cast_align_crypto_hash

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8617
Build 15221: Default Diff Build & Tests
Build 15220: arc lint + arc unit

Event Timeline

Fabien created this revision.Dec 10 2019, 10:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 10 2019, 10:21

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8472: Diff 14721.Dec 10 2019, 10:32

deadalnix requested changes to this revision.Dec 11 2019, 01:04

deadalnix added inline comments.

src/bench/crypto_hash.cpp
74 ↗	(On Diff #14721)	Why not make x part of the union instead?

This revision now requires changes to proceed.Dec 11 2019, 01:04

Fabien mentioned this in D4731: We assume uint8_t is an alias of unsigned char..Dec 17 2019, 10:13

Fabien edited the summary of this revision. (Show Details)Dec 17 2019, 14:12

Fabien edited the test plan for this revision. (Show Details)

Use an uint256 in the union, take care of avoiding UB.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Use std::memcpy instead of memcpy and <string> instead of <cstring>.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8593: Diff 14908.Dec 17 2019, 14:24

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8594: Diff 14909.Dec 17 2019, 14:29

Memcpy to overlapping memory is also UB right?

BTW what's wrong with just using non-union?

uint64_t y = SipHashUint256(0, ++k1, x);
std::memcpy(&x, &y, sizeof(y));

is the compiler unable to optimize that? (and, does it really matter if it's one extra instruction where y is probably a register?)

deadalnix requested changes to this revision.Dec 17 2019, 23:18

deadalnix added inline comments.

src/bench/crypto_hash.cpp
82 ↗	(On Diff #14909)	memcpy is undefined when source and destination overlap, so you made it worse. You also end up so setting the higher bytes of the hash.u256, so you do not avoid the UB you where trying to avoid. Maybe you just want to drop the union, and use memcpy from a local variable.

This revision now requires changes to proceed.Dec 17 2019, 23:18

Fabien added inline comments.Dec 18 2019, 08:08

src/bench/crypto_hash.cpp
82 ↗	(On Diff #14909)	Yes I'll stop toying with unions. Out of curiosity, why is it writing the higher bytes ? If I'm referring to https://en.cppreference.com/w/cpp/language/union the addresses are the same for the members of the union.