Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin

paveljanik · 2016-09-25T17:22:59Z

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.

paveljanik · 2016-09-25T19:08:20Z

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

maflcko · 2016-09-26T07:27:41Z

Travis fails reliably. Is the commit producing identical binaries?

paveljanik · 2016-09-26T15:25:15Z

This commit should produce same binaries, yes.

paveljanik · 2016-09-26T19:54:25Z

src/primitives/block.h

Hmm, this particular change is causing my clang on OS X build to fail -rescan with:

2016-09-26 19:53:08 ERROR: ReadBlockFromDisk: Deserialize or I/O error - ReadCompactSize(): size too large: unspecified iostream_category error at CBlockDiskPos(nFile=49, nPos=4208853)

So reverting it.

But do not ask me, why it is the cause... I do not know yet.

I think I figured this out, see #8658 (comment) and the comment below that.

laanwj · 2016-09-28T08:16:13Z

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

paveljanik · 2016-10-02T17:44:53Z

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

paveljanik · 2016-10-07T14:17:41Z

Rebased.

paveljanik · 2016-11-07T12:06:10Z

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

maflcko · 2016-11-07T13:03:53Z

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

paveljanik · 2016-11-07T13:28:46Z

This seems unrelated.

fanquake · 2016-11-09T14:12:19Z

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

paveljanik · 2016-11-09T14:17:58Z

@fanquake surely not...

paveljanik · 2016-11-09T14:26:25Z

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

gmaxwell · 2016-11-09T21:45:55Z

src/arith_uint256.cpp

change _bits to nbits.

maflcko · 2016-11-09T21:47:08Z

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

gmaxwell

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.

gmaxwell · 2016-11-09T21:46:14Z

src/streams.h

change _read to nBytes

gmaxwell · 2016-11-09T21:48:08Z

src/timedata.h

vSortedSize ?

I like vSortedSize, will do.

gmaxwell · 2016-11-09T21:51:18Z

src/primitives/transaction.h

sflags (this was less obvious to me, but my suggestion and @sipa thought it was okay)

serializeFlags?

I like both.

gmaxwell · 2016-11-09T21:57:27Z

src/wallet/wallet.cpp

So I believe changing the name of CCryptoKeyStore master key will, I think change only four lines. Might be good to get an opinion from someone who's worked more on this code? @TheBlueMatt @pstratem

That would be great and even better solution, yes. Will wait.

gmaxwell · 2016-11-09T22:10:12Z

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.

paveljanik · 2016-11-10T07:02:54Z

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.

maflcko · 2016-11-10T10:35:59Z

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

ad1ae7a Check and enable -Wshadow by default. (Pavel Janík) 9de90bb Do not shadow variables (gcc set) (Pavel Janík) Tree-SHA512: 9517feb423dc8ddd63896016b25324673bfbe0bffa97f22996f59d7a3fcbdc2ebf2e43ac02bc067546f54e293e9b2f2514be145f867321e9031f895c063d9fb8

paveljanik force-pushed the 20160925_Wshadow_gcc branch from d7036e6 to d7690b3 Compare September 25, 2016 17:46

dcousens approved these changes Sep 26, 2016

View reviewed changes

paveljanik commented Sep 26, 2016

View reviewed changes

maflcko mentioned this pull request Sep 26, 2016

Do not shadow variables (trivials) #8655

Merged

paveljanik mentioned this pull request Sep 28, 2016

Remove unused statements in serialization #8658

Merged

laanwj added the Refactoring label Sep 30, 2016

paveljanik force-pushed the 20160925_Wshadow_gcc branch 2 times, most recently from fca66b4 to dd08dc7 Compare October 2, 2016 17:44

paveljanik force-pushed the 20160925_Wshadow_gcc branch from dd08dc7 to 416654b Compare October 7, 2016 14:17

This was referenced Nov 9, 2016

Enable -Wshadow by default #8794

Merged

avoid shadowing vars, and enable -Wshadow #8105

Closed

gmaxwell reviewed Nov 9, 2016

View reviewed changes

paveljanik force-pushed the 20160925_Wshadow_gcc branch from 416654b to bedf9a7 Compare November 10, 2016 07:01

laanwj mentioned this pull request Apr 1, 2017

build: Disable Wshadow warning #10136

Merged

str4d mentioned this pull request Jan 29, 2020

util: CBufferedFile fixes zcash/zcash#4265

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

paveljanik commented Sep 25, 2016

Uh oh!

paveljanik commented Sep 25, 2016

Uh oh!

maflcko commented Sep 26, 2016

Uh oh!

paveljanik commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Oct 2, 2016

Uh oh!

paveljanik commented Oct 7, 2016

Uh oh!

paveljanik commented Nov 7, 2016

Uh oh!

maflcko commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paveljanik commented Nov 7, 2016

Uh oh!

fanquake commented Nov 9, 2016

Uh oh!

paveljanik commented Nov 9, 2016

Uh oh!

paveljanik commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Nov 9, 2016

Uh oh!

paveljanik commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

laanwj commented Sep 28, 2016 •

edited

Loading

maflcko commented Nov 7, 2016 •

edited

Loading

maflcko commented Nov 9, 2016 •

edited

Loading

paveljanik commented Nov 10, 2016 •

edited

Loading