Do not shadow variables (trivials) by paveljanik · Pull Request #8655 · bitcoin/bitcoin

paveljanik · 2016-09-02T16:27:40Z

This is a followup to #8105.

More trivial changes merged into one PR.

maflcko · 2016-09-04T12:28:22Z

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Also, I was wondering how much is left until we can enable Wshadow

paveljanik · 2016-09-04T12:31:09Z

I can squash when the review is finished, no problem. I think it is much easier reviewing separately.

maflcko · 2016-09-04T21:14:54Z

@paveljanik The diff is just 20 lines and I think trying to compile before/after and compare the objdump is sufficient to review. (Maybe skim over the diff once)
This is achieved easily when there is only one commit.

paveljanik · 2016-09-05T05:09:58Z

Squashed.

paveljanik · 2016-09-15T07:28:29Z

Please review.

maflcko · 2016-09-15T10:25:39Z

src/reverselock.h

I think this patch changes the binaries.

Interesting. I do not see why it should change binaries, but will investigate.

Wladimir investigated in #8793. It changes the binaries.

Technically it can be done by changing lock to _lock in the body of the function. But we decided that it is wrong to do that.

Well in the case of the GUI that was the decision, as an exception.

As I said there, for the consensus code I would require the binaries to be the same. This is much closer to that.

OK then, going to add squashme commit fixing this.

Well, I don't want to redo the check a third time when you squash.

I understand this ;-)

Thanks. Though, it seems I still don't get identical binaries. Could you try to create a pull with a single commit and combine #8655 + #8808 with all the changes that don't change the binary? Then we could evaluate the remaining (problematic) hunks separate from those.

@laanwj Can ask you to do your different-binary-function analysis on this PR? Or better: can you publish your tool? ;-)

I'll take a look. Aand maybe release the script, though it's a Chthonic monstriosity with tentacles spreading to other unpublished tools, so I have to figure out what to collect along the way.

paveljanik · 2016-09-19T13:15:20Z

pagelocker going to be removed in #8753...

paveljanik · 2016-09-26T15:10:06Z

Squashed.

laanwj · 2016-09-27T06:50:16Z

Two functions differ:

LockedPageManagerBase<MemoryPageLocker>::LockedPageManagerBase(unsigned long)
TorController::TorController(event_base*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)

From a quick look at the assembly differences it looks like the same problem as before.

paveljanik · 2016-09-27T07:02:15Z

The first one is clear (page_size in the body to _page_size). The second one target -> _target.
Both are stupid to be done IMO 8)

paveljanik · 2016-09-27T07:25:49Z

But done... And squashed. The diff is a bit longer now.

maflcko · 2016-09-27T09:00:47Z

Identical binaries ACK 4731cab

fanquake · 2016-09-27T09:49:12Z

@MarcoFalke What tool are you using to generate the identical binary results?

laanwj · 2016-09-27T10:01:07Z

I've put up my script here: https://github.com/laanwj/bitcoin-maintainer-tools

BTW: I don't get identical bitcoind binaries for 2f71490 and 4731cab but don't see any function-level differences, will investigate further.

laanwj · 2016-09-27T10:15:12Z

OK, found the reason:

-2f71490.o/src/support/libbitcoin_util_a-pagelocker.o:     file format elf64-x86-64
+4731cab.o/src/support/libbitcoin_util_a-pagelocker.o:     file format elf64-x86-64

 Contents of section .group:
  0000 01000000 57000000                    ....W...        
@@ -314,9 +314,9 @@
  0000 626f6f73 743a3a20 6d757465 7820636f  boost:: mutex co
  0010 6e737472 7563746f 72206661 696c6564  nstructor failed
  0020 20696e20 70746872 6561645f 6d757465   in pthread_mute
- 0030 785f696e 69740000 21287061 67655f73  x_init..!(page_s
- 0040 697a6520 26202870 6167655f 73697a65  ize & (page_size
- 0050 202d2031 292900                       - 1)).         
+ 0030 785f696e 69740000 21285f70 6167655f  x_init..!(_page_
+ 0040 73697a65 20262028 5f706167 655f7369  size & (_page_si
+ 0050 7a65202d 20312929 00                 ze - 1)).

This is a string emitted by an assertion. You're never going to get this right.
(building without assertions would get identical binaries though, that's probably what @MarcoFalke did)
I can also confirm binaries are identical w/out assertions.

ACK 4731cab

4731cab Do not shadow variables (Pavel Janík)

maflcko · 2016-09-27T12:13:42Z

Huh, I did not disable assertions. Just building as usual and making sure that the clientversion does not influence the binaries. Then calling objdump -d and diff the output.

laanwj · 2016-09-27T13:14:35Z

Then calling objdump -d and diff the output.

Ok, that explains it, because that doesn't detect changes in data sections. I always diff the assembly as well as comparing sha256sums of the stripped executables just in case.

4731cab Do not shadow variables (Pavel Janík)

Backport bloom filter improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7113 - bitcoin/bitcoin#7818 - Only the second commit (to resolve conflicts). - bitcoin/bitcoin#7934 - bitcoin/bitcoin#8655 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9060 - bitcoin/bitcoin#9223 - bitcoin/bitcoin#9644 - Partial backport to help resolve conflicts. - bitcoin/bitcoin#9916 - bitcoin/bitcoin#9750 - bitcoin/bitcoin#13176 - bitcoin/bitcoin#13948 - bitcoin/bitcoin#16073 - bitcoin/bitcoin#18670 - bitcoin/bitcoin#18806 - Reveals upstream's covert fix for CVE-2013-5700. - bitcoin/bitcoin#19968

fanquake added the Refactoring label Sep 3, 2016

paveljanik force-pushed the 20160902_Wshadow_trivials branch from a023a05 to d3eff19 Compare September 5, 2016 05:09

maflcko suggested changes Sep 15, 2016

View reviewed changes

maflcko approved these changes Sep 15, 2016

View reviewed changes

paveljanik force-pushed the 20160902_Wshadow_trivials branch 2 times, most recently from 93dd3c7 to ca6bf63 Compare September 23, 2016 14:09

paveljanik mentioned this pull request Sep 23, 2016

avoid shadowing vars, and enable -Wshadow #8105

Closed

8 tasks

paveljanik force-pushed the 20160902_Wshadow_trivials branch from 89df412 to a413d9d Compare September 26, 2016 15:07

Do not shadow variables

4731cab

paveljanik force-pushed the 20160902_Wshadow_trivials branch from a413d9d to 4731cab Compare September 27, 2016 07:25

laanwj merged commit 4731cab into bitcoin:master Sep 27, 2016

laanwj added a commit that referenced this pull request Sep 27, 2016

Merge #8655: Do not shadow variables (trivials)

920ca1f

4731cab Do not shadow variables (Pavel Janík)

laanwj mentioned this pull request Sep 27, 2016

Enable -Wshadow by default #8794

Merged

laanwj mentioned this pull request Dec 2, 2016

0.13.2 backports #9264

Merged

codablock pushed a commit to codablock/dash that referenced this pull request Jan 11, 2018

Merge bitcoin#8655: Do not shadow variables (trivials)

ec9de93

4731cab Do not shadow variables (Pavel Janík)

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#8655: Do not shadow variables (trivials)

60615b3

4731cab Do not shadow variables (Pavel Janík)

str4d mentioned this pull request Mar 5, 2021

Backport bloom filter improvements zcash/zcash#5026

Merged

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

Conversation

paveljanik commented Sep 2, 2016

Uh oh!

maflcko commented Sep 4, 2016

Uh oh!

paveljanik commented Sep 4, 2016

Uh oh!

maflcko commented Sep 4, 2016

Uh oh!

paveljanik commented Sep 5, 2016

Uh oh!

paveljanik commented Sep 15, 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!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Sep 19, 2016

Uh oh!

paveljanik commented Sep 26, 2016

Uh oh!

laanwj commented Sep 27, 2016

Uh oh!

paveljanik commented Sep 27, 2016

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Sep 27, 2016

Uh oh!

fanquake commented Sep 27, 2016

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Sep 27, 2016

Uh oh!

laanwj commented Sep 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paveljanik Sep 26, 2016 •

edited

Loading

paveljanik commented Sep 27, 2016 •

edited

Loading

laanwj commented Sep 27, 2016 •

edited

Loading

laanwj commented Sep 27, 2016 •

edited

Loading