Page MenuHomePhabricator

Fix -Wcast-align in crypto_hash.cpp
ClosedPublic

Authored by Fabien on Dec 10 2019, 10:21.

Details

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 8632
Build 15250: Default Diff Build & Tests
Build 15249: arc lint + arc unit

Event Timeline

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 edited the test plan for this revision. (Show Details)
Fabien edited the test plan for this revision. (Show Details)

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

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

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
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.

deadalnix added inline comments.
src/bench/crypto_hash.cpp
68 ↗(On Diff #14956)

Declare this locally.

This revision is now accepted and ready to land.Dec 18 2019, 17:12
This revision was automatically updated to reflect the committed changes.