Do not shadow variables (trivials) #8655

paveljanik commented at 4:27 PM on September 2, 2016: contributor

This is a followup to #8105.

More trivial changes merged into one PR.

fanquake added the label Refactoring on Sep 3, 2016

MarcoFalke commented at 12:28 PM on September 4, 2016: member

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 commented at 12:31 PM on September 4, 2016: contributor

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

MarcoFalke commented at 9:14 PM on September 4, 2016: member

@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 force-pushed on Sep 5, 2016

paveljanik commented at 5:09 AM on September 5, 2016: contributor

Squashed.

paveljanik commented at 7:28 AM on September 15, 2016: contributor

Please review.

in src/reverselock.h:None in d3eff19fc0 outdated

  12 | @@ -13,7 +13,7 @@ class reverse_lock
  13 |  {
  14 |  public:
  15 |  
  16 | -    explicit reverse_lock(Lock& lock) : lock(lock) {
  17 | +    explicit reverse_lock(Lock& _lock) : lock(_lock) {

MarcoFalke commented at 10:25 AM on September 15, 2016:

I think this patch changes the binaries.

paveljanik commented at 11:35 AM on September 15, 2016:

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

paveljanik commented at 6:08 PM on September 22, 2016:

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.

laanwj commented at 1:48 PM on September 26, 2016:

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.

paveljanik commented at 2:48 PM on September 26, 2016:

OK then, going to add squashme commit fixing this.

MarcoFalke commented at 2:52 PM on September 26, 2016:

Can you also squash it? Otherwise no one can start review to check if the binaries are the same.

paveljanik commented at 2:55 PM on September 26, 2016:

Why you can't start to check if it is not squashed? I do not understand...

paveljanik commented at 2:56 PM on September 26, 2016:

Having the squashme commit clearly marked enables us to discuss the change needed.

paveljanik commented at 2:58 PM on September 26, 2016:

And if you need all-in-one diff, you can use https://patch-diff.githubusercontent.com/raw/bitcoin/bitcoin/pull/8655.diff

paveljanik commented at 3:00 PM on September 26, 2016:

In this particular case, the requirement for same-binary makes the diff longer than needed. But I agree that in this case, it is better to have the same binary just to be sure.

MarcoFalke commented at 3:01 PM on September 26, 2016:

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

paveljanik commented at 3:07 PM on September 26, 2016:

I understand this ;-)

MarcoFalke commented at 8:03 PM on September 26, 2016:

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.

paveljanik commented at 8:11 PM on September 26, 2016:

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

laanwj commented at 6:46 AM on September 27, 2016:

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.

MarcoFalke changes_requested

MarcoFalke approved

paveljanik commented at 1:15 PM on September 19, 2016: contributor

pagelocker going to be removed in #8753...

paveljanik force-pushed on Sep 23, 2016

paveljanik force-pushed on Sep 26, 2016

paveljanik commented at 3:10 PM on September 26, 2016: contributor

Squashed.

laanwj commented at 6:50 AM on September 27, 2016: member

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 commented at 7:02 AM on September 27, 2016: contributor

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)

Do not shadow variables 4731cab8fb

paveljanik force-pushed on Sep 27, 2016

paveljanik commented at 7:25 AM on September 27, 2016: contributor

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

MarcoFalke commented at 9:00 AM on September 27, 2016: member

Identical binaries ACK 4731cab

fanquake commented at 9:49 AM on September 27, 2016: member

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

laanwj commented at 10:01 AM on September 27, 2016: member

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 commented at 10:15 AM on September 27, 2016: member

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 https://github.com/bitcoin/bitcoin/pull/8655/commits/4731cab8fbff51a8178c85d572e2482040278616

laanwj merged this on Sep 27, 2016

laanwj closed this on Sep 27, 2016

laanwj referenced this in commit 920ca1f0bf on Sep 27, 2016

MarcoFalke commented at 12:13 PM on September 27, 2016: member

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 commented at 1:14 PM on September 27, 2016: member

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.

codablock referenced this in commit d3ce11117f on Sep 19, 2017

codablock referenced this in commit ec9de93aee on Jan 11, 2018

andvgal referenced this in commit 60615b3f3e on Jan 6, 2019

zkbot referenced this in commit be459619a8 on Mar 5, 2021

zkbot referenced this in commit 78de0cdf46 on Apr 15, 2021

MarcoFalke locked this on Sep 8, 2021