Do not shadow variables (gcc set) #8808

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

@@ -92,7 +92,7 @@ class CBlock : public CBlockHeader
    ADD_SERIALIZE_METHODS;

    template <typename Stream, typename Operation>
-    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int _nVersion) {
+    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {


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

@@ -173,9 +173,9 @@ unsigned int base_uint<BITS>::bits() const
 {
    for (int pos = WIDTH - 1; pos >= 0; pos--) {
        if (pn[pos]) {
-            for (int bits = 31; bits > 0; bits--) {


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

            throw std::ios_base::failure(feof(src) ? "CBufferedFile::Fill: end of file" : "CBufferedFile::Fill: fread failed");
        } else {
-            nSrcPos += read;
+            nSrcPos += _read;


change _read to nBytes

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

src/timedata.h

-        int size = vSorted.size();
-        assert(size > 0);
-        if (size & 1) // Odd number of elements
+        int _size = vSorted.size();


vSortedSize ?

I like vSortedSize, will do.

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

src/primitives/transaction.h

@@ -293,7 +293,7 @@ inline void SerializeTransaction(TxType& tx, Stream& s, Operation ser_action, in
    const bool fAllowWitness = !(nVersion & SERIALIZE_TRANSACTION_NO_WITNESS);

    READWRITE(*const_cast<int32_t*>(&tx.nVersion));
-    unsigned char flags = 0;
+    unsigned char _flags = 0;


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

@@ -280,17 +280,17 @@ bool CWallet::LoadWatchOnly(const CScript &dest)
 bool CWallet::Unlock(const SecureString& strWalletPassphrase)
 {
    CCrypter crypter;
-    CKeyingMaterial vMasterKey;
+    CKeyingMaterial _vMasterKey;


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

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 21, 2019

fix bitcoin#8808 backport

1f764bc

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 25, 2019

fix bitcoin#8808 backport

7ec4cd4

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2019

fix bitcoin#8808 backport

6797b96

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Feb 1, 2019

fix bitcoin#8808 backport

ead4559

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not shadow variables (gcc set) #8808

Do not shadow variables (gcc set) #8808

paveljanik commented Sep 25, 2016

paveljanik commented Sep 25, 2016

maflcko commented Sep 26, 2016

paveljanik commented Sep 26, 2016

paveljanik Sep 26, 2016

laanwj Sep 28, 2016

laanwj commented Sep 28, 2016 •

edited

Loading

paveljanik commented Oct 2, 2016

paveljanik commented Oct 7, 2016

paveljanik commented Nov 7, 2016

maflcko commented Nov 7, 2016 •

edited

Loading

paveljanik commented Nov 7, 2016

fanquake commented Nov 9, 2016

paveljanik commented Nov 9, 2016

paveljanik commented Nov 9, 2016

gmaxwell Nov 9, 2016

paveljanik Nov 10, 2016

paveljanik Nov 10, 2016

maflcko commented Nov 9, 2016 •

edited

Loading

gmaxwell left a comment

gmaxwell Nov 9, 2016

paveljanik Nov 10, 2016

paveljanik Nov 10, 2016

gmaxwell Nov 9, 2016

dcousens Nov 10, 2016

paveljanik Nov 10, 2016

gmaxwell Nov 9, 2016

dcousens Nov 10, 2016

paveljanik Nov 10, 2016

gmaxwell Nov 9, 2016

paveljanik Nov 10, 2016

gmaxwell commented Nov 9, 2016

paveljanik commented Nov 10, 2016 •

edited

Loading

maflcko commented Nov 10, 2016

Do not shadow variables (gcc set) #8808

Do not shadow variables (gcc set) #8808

Conversation

paveljanik commented Sep 25, 2016

paveljanik commented Sep 25, 2016

maflcko commented Sep 26, 2016

paveljanik commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Sep 28, 2016 • edited Loading

paveljanik commented Oct 2, 2016

paveljanik commented Oct 7, 2016

paveljanik commented Nov 7, 2016

maflcko commented Nov 7, 2016 • edited Loading

paveljanik commented Nov 7, 2016

fanquake commented Nov 9, 2016

paveljanik commented Nov 9, 2016

paveljanik commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maflcko commented Nov 9, 2016 • edited Loading

gmaxwell left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Nov 9, 2016

paveljanik commented Nov 10, 2016 • edited Loading

maflcko commented Nov 10, 2016

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