Do not shadow variables (gcc set) #8808

Followup to #8105.

The current set of PRs made clang compile Wshadow clean. Compiling with gcc revealed new needed changes. gcc is more strict in warnings. This is non-Qt part. I do not have an environment to test gcc+Qt build now.

Hmm, two failures in p2p-segwit.py. Is there some issue with this test script?

Travis fails reliably. Is the commit producing identical binaries?

This commit should produce same binaries, yes.

This introduces binary changes in these serialization functions:

void CDiskBlockIndex::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CDiskBlockIndex::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CDiskBlockIndex::SerializationOp<CSizeComputer, CSerActionSerialize>(CSizeComputer&, CSerActionSerialize, int, int)
void CMerkleTx::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CMerkleTx::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)
void CWalletTx::SerializationOp<CDataStream, CSerActionSerialize>(CDataStream&, CSerActionSerialize, int, int)
void CWalletTx::SerializationOp<CDataStream, CSerActionUnserialize>(CDataStream&, CSerActionUnserialize, int, int)

Edit: and in contrast to #8658, these do not go away with -O2

OK, rebased and removed problematic parts (all serialization related, nVersion)...

Rebased.

@MarcoFalke Marco, can you please test for the same binaries here?

There seems to be a small diff:

319294c319294
<   17d0f1: lea    0x2dd1b2(%rip),%rdi        # 45a2aa <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xaa>
---
>   17d0f1: lea    0x2dd1b1(%rip),%rdi        # 45a2a9 <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xa9>

used the following command:

commit=416654b4bdcc9f91cf84d4e3c645826d62979c1d && git checkout bitcoin/master && git reset --hard HEAD && curl https://raw.githubusercontent.com/laanwj/bitcoin-maintainer-tools/6e4425587736144b067f67ad792d9ee904e74fd7/patches/stripbuildinfo.patch | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_old && curl https://github.com/bitcoin/bitcoin/commit/"${commit}".diff | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_new && diff /tmp/d_old /tmp/d_new | wc

This seems unrelated.

I haven't done the identical binary checking yet, but it looks like the difference could be here ? 170 (0xaa) vs 169 (0xa9)

@fanquake surely not...

JFYI: I'm now building with other gcc version and there are a few other warnings 8)

Concept ACK when this is the last patch/pull with regard to Wshadow.

filling up locals with _ prefixes in big pieces of code is less good than using better names, and also incompatible with a common convention of using _prefixes for function parameters.

I checked each of the changes so far, they appear correct to me (+/- the suggestion I made about changing the name of the member vMasterKey that I asked for others input on). I didn't check that they resulted in the same code.

Rebased, updated.

It is now -Wshadow clean for both old gcc (tested on gcc version 4.8.3 20140627 [gcc-4_8-branch revision 212064](SUSE Linux)) and also on the ultimate gcc (gcc version 6.2.1 20160830 [gcc-6-branch revision 239856](SUSE Linux)). And of course stays clean on other systems I regularly test on.

@paveljanik Mind to cherry-pick fd5654c, so we don't have to open another pull?

I get same binaries for bedf9a7 besides this diff:

commit=bedf9a7 && git checkout bitcoin/master && git reset --hard HEAD && curl https://raw.githubusercontent.com/laanwj/bitcoin-maintainer-tools/6e4425587736144b067f67ad792d9ee904e74fd7/patches/stripbuildinfo.patch | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_old && curl https://github.com/bitcoin/bitcoin/commit/"${commit}".diff | patch -p 1 && make -j 2 && objdump -d -r -C --no-show-raw-insn src/bitcoind > /tmp/d_new && diff /tmp/d_old /tmp/d_new

317267c317267
<   17aa0b: lea    0x25704b(%rip),%rsi        # 3d1a5d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0xfd>
---
>   17aa0b: lea    0x25705b(%rip),%rsi        # 3d1a6d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x10d>
317684c317684
<   17b325: lea    0x256737(%rip),%rsi        # 3d1a63 <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x103>
---
>   17b325: lea    0x256747(%rip),%rsi        # 3d1a73 <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0x113>
317757c317757
<   17b471: lea    0x2dc552(%rip),%rdi        # 4579ca <leveldb::Status::Status(leveldb::Status::Code, leveldb::Slice const&, leveldb::Slice const&)::__PRETTY_FUNCTION__+0xaa>
---
>   17b471: lea    0x2565e5(%rip),%rdi        # 3d1a5d <boost::recursive_mutex::lock()::__PRETTY_FUNCTION__+0xfd>

@MarcoFalke I can, but Wladimir doesn't want it to be enabled by default...

utACK f14745d

Either the warning is enabled and people can fix the warnings when they write new code or there is no warning and we stop caring about it. There is no such thing as "disable the warnign by default and create refactoring pulls every other week to fix new occurences".

To conclude, I think this is ready for merge. If it turns out to cause problems in the near future, we disable it and call it done.

@MarcoFalke Agreed. We should merge this and then fix the remaining issues found by users who use other build environments/configurations.

Concept ACK

Working on needed rebase...

utACK ad1ae7ae2e90a29fcfaaddee2d5acd025de72cbe, 9de90bb gives same binaries for me besides some boost_pretty_functions.

utACK ad1ae7a

Checked that this, rebased on master, results in identical bitcoind executables. ACK ad1ae7a