refactor: Replace memset calls with array initialization #21939

promag · 2021-05-13T07:43:01Z

Follow up to #21905 (review).

laanwj · 2021-05-13T08:53:36Z

re-ACK 1c9255c

Checked the disassembly with -O0:

00000000000c0834 <CMessageHeader::CMessageHeader()>:
   c0834:       f3 0f 1e fa             endbr64 
   c0838:       55                      push   %rbp
   c0839:       48 89 e5                mov    %rsp,%rbp
   c083c:       48 83 ec 20             sub    $0x20,%rsp
   c0840:       48 89 7d e8             mov    %rdi,-0x18(%rbp)
   c0844:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
   c084b:       00 00 
   c084d:       48 89 45 f8             mov    %rax,-0x8(%rbp)
   c0851:       31 c0                   xor    %eax,%eax
   c0853:       48 8b 45 e8             mov    -0x18(%rbp),%rax
   c0857:       c7 00 00 00 00 00       movl   $0x0,(%rax)
   c085d:       48 8b 45 e8             mov    -0x18(%rbp),%rax
   c0861:       48 c7 40 04 00 00 00    movq   $0x0,0x4(%rax)
   c0868:       00 
   c0869:       c7 40 0c 00 00 00 00    movl   $0x0,0xc(%rax)
   c0870:       e8 41 b1 f9 ff          callq  5b9b6 <std::numeric_limits<unsigned int>::max()>
   c0875:       89 c2                   mov    %eax,%edx
   c0877:       48 8b 45 e8             mov    -0x18(%rbp),%rax
   c087b:       89 50 10                mov    %edx,0x10(%rax)
   c087e:       48 8b 45 e8             mov    -0x18(%rbp),%rax
   c0882:       c7 40 14 00 00 00 00    movl   $0x0,0x14(%rax)
   c0889:       90                      nop
   c088a:       48 8b 45 f8             mov    -0x8(%rbp),%rax
   c088e:       64 48 33 04 25 28 00    xor    %fs:0x28,%rax
   c0895:       00 00 
   c0897:       74 05                   je     c089e <CMessageHeader::CMessageHeader()+0x6a>
   c0899:       e8 72 cd f6 ff          callq  2d610 <__stack_chk_fail@plt>
   c089e:       c9                      leaveq 
   c089f:       c3                      retq

It doesn't generate memsets, but the mov $0, XXX are correct:

   4@0x00   pchMessageStart[0..3]
   c0857:       c7 00 00 00 00 00       movl   $0x0,(%rax)

   8@0x04   pchCommand[0..7]
   c0861:       48 c7 40 04 00 00 00    movq   $0x0,0x4(%rax)
   c0868:       00 

   4@0x0c   pchCommand[8..11]
   c0869:       c7 40 0c 00 00 00 00    movl   $0x0,0xc(%rax)

   4@0x14   pchChecksum[0..3]
   c0882:       c7 40 14 00 00 00 00    movl   $0x0,0x14(%rax)

Second constructor

Details

000000000083007c <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)>:
  83007c:       f3 0f 1e fa             endbr64 
  830080:       55                      push   %rbp
  830081:       48 89 e5                mov    %rsp,%rbp
  830084:       48 83 ec 30             sub    $0x30,%rsp
  830088:       48 89 7d e8             mov    %rdi,-0x18(%rbp)
  83008c:       48 89 75 e0             mov    %rsi,-0x20(%rbp)
  830090:       48 89 55 d8             mov    %rdx,-0x28(%rbp)
  830094:       89 4d d4                mov    %ecx,-0x2c(%rbp)
  830097:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
  83009e:       00 00 
  8300a0:       48 89 45 f8             mov    %rax,-0x8(%rbp)
  8300a4:       31 c0                   xor    %eax,%eax
  8300a6:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  8300aa:       c7 00 00 00 00 00       movl   $0x0,(%rax)
  8300b0:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  8300b4:       48 c7 40 04 00 00 00    movq   $0x0,0x4(%rax)
  8300bb:       00 
  8300bc:       c7 40 0c 00 00 00 00    movl   $0x0,0xc(%rax)
  8300c3:       e8 ee b8 82 ff          callq  5b9b6 <std::numeric_limits<unsigned int>::max()>
  8300c8:       89 c2                   mov    %eax,%edx
  8300ca:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  8300ce:       89 50 10                mov    %edx,0x10(%rax)
  8300d1:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  8300d5:       c7 40 14 00 00 00 00    movl   $0x0,0x14(%rax)
  8300dc:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  8300e0:       48 8b 55 e0             mov    -0x20(%rbp),%rdx
  8300e4:       8b 12                   mov    (%rdx),%edx
  8300e6:       89 10                   mov    %edx,(%rax)
  8300e8:       48 c7 45 f0 00 00 00    movq   $0x0,-0x10(%rbp)
  8300ef:       00 
  8300f0:       48 83 7d f0 0b          cmpq   $0xb,-0x10(%rbp)
  8300f5:       77 38                   ja     83012f <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0xb3>
  8300f7:       48 8b 55 d8             mov    -0x28(%rbp),%rdx
  8300fb:       48 8b 45 f0             mov    -0x10(%rbp),%rax
  8300ff:       48 01 d0                add    %rdx,%rax
  830102:       0f b6 00                movzbl (%rax),%eax
  830105:       84 c0                   test   %al,%al
  830107:       74 26                   je     83012f <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0xb3>
  830109:       48 8b 55 d8             mov    -0x28(%rbp),%rdx
  83010d:       48 8b 45 f0             mov    -0x10(%rbp),%rax
  830111:       48 01 d0                add    %rdx,%rax
  830114:       0f b6 00                movzbl (%rax),%eax
  830117:       48 8b 4d e8             mov    -0x18(%rbp),%rcx
  83011b:       48 8b 55 f0             mov    -0x10(%rbp),%rdx
  83011f:       48 01 ca                add    %rcx,%rdx
  830122:       48 83 c2 04             add    $0x4,%rdx
  830126:       88 02                   mov    %al,(%rdx)
  830128:       48 83 45 f0 01          addq   $0x1,-0x10(%rbp)
  83012d:       eb c1                   jmp    8300f0 <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0x74>
  83012f:       48 8b 55 d8             mov    -0x28(%rbp),%rdx
  830133:       48 8b 45 f0             mov    -0x10(%rbp),%rax
  830137:       48 01 d0                add    %rdx,%rax
  83013a:       0f b6 00                movzbl (%rax),%eax
  83013d:       84 c0                   test   %al,%al
  83013f:       74 1f                   je     830160 <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0xe4>
  830141:       48 8d 0d 58 69 28 00    lea    0x286958(%rip),%rcx        # ab6aa0 <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)::__PRETTY_FUNCTION__>
  830148:       ba 61 00 00 00          mov    $0x61,%edx
  83014d:       48 8d 35 79 67 28 00    lea    0x286779(%rip),%rsi        # ab68cd <DEFAULT_LOGSOURCELOCATIONS+0x136>
  830154:       48 8d 3d 81 67 28 00    lea    0x286781(%rip),%rdi        # ab68dc <DEFAULT_LOGSOURCELOCATIONS+0x145>
  83015b:       e8 40 d2 7f ff          callq  2d3a0 <__assert_fail@plt>
  830160:       48 83 7d f0 0b          cmpq   $0xb,-0x10(%rbp)
  830165:       77 19                   ja     830180 <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0x104>
  830167:       48 8b 55 e8             mov    -0x18(%rbp),%rdx
  83016b:       48 8b 45 f0             mov    -0x10(%rbp),%rax
  83016f:       48 01 d0                add    %rdx,%rax
  830172:       48 83 c0 04             add    $0x4,%rax
  830176:       c6 00 00                movb   $0x0,(%rax)
  830179:       48 83 45 f0 01          addq   $0x1,-0x10(%rbp)
  83017e:       eb e0                   jmp    830160 <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0xe4>
  830180:       48 8b 45 e8             mov    -0x18(%rbp),%rax
  830184:       8b 55 d4                mov    -0x2c(%rbp),%edx
  830187:       89 50 10                mov    %edx,0x10(%rax)
  83018a:       90                      nop
  83018b:       48 8b 45 f8             mov    -0x8(%rbp),%rax
  83018f:       64 48 33 04 25 28 00    xor    %fs:0x28,%rax
  830196:       00 00 
  830198:       74 05                   je     83019f <CMessageHeader::CMessageHeader(unsigned char const (&) [4], char const*, unsigned int)+0x123>
  83019a:       e8 71 d4 7f ff          callq  2d610 <__stack_chk_fail@plt>
  83019f:       c9                      leaveq 
  8301a0:       c3                      retq

This one is more complex, but it works out, contains the same clearing code:

  8300aa:       c7 00 00 00 00 00       movl   $0x0,(%rax)
  8300b4:       48 c7 40 04 00 00 00    movq   $0x0,0x4(%rax)
  8300bb:       00 
  8300bc:       c7 40 0c 00 00 00 00    movl   $0x0,0xc(%rax)
  8300d5:       c7 40 14 00 00 00 00    movl   $0x0,0x14(%rax)

promag · 2021-05-13T10:14:53Z

For reference, from https://en.cppreference.com/w/c/language/array_initialization

All array elements that are not initialized explicitly are zero-initialized.

kristapsk

utACK 5d2e40c4d2394758e8026f796de89e8569f8dc86, code looks correct.

src/protocol.cpp

laanwj · 2021-05-13T14:42:02Z

src/protocol.cpp

-    memset(pchChecksum, 0, CHECKSUM_SIZE);
-}
-
 CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)


We could also change the type here to std::string or std::span instead of a loose pointer, at some point and clean up the loop (oh and rename the argument to message_type maybe instead of command, when we're at it). No need to do so in this PR though.

Crypt-iQ · 2021-05-13T14:47:33Z

Code review ACK 1c9255c

Looking at the code I noticed both pchCommand, pchMessageStart are char arrays. There is a memcpy done from MessageStartChars (unsigned char array) to pchMessageStart, and some architectures specify char as signed. May be a case for explicitly making them unsigned char arrays?

maflcko · 2021-05-13T16:03:34Z

I have a patchset to remove char, but haven't pushed it yet

laanwj · 2021-05-13T16:54:19Z

Looking at the code I noticed both pchCommand, pchMessageStart are char arrays.

Another good point. There are only very few places where bare 'char' is a good choice. But let's leave it for a future PR.

…lization 1c9255c refactor: Replace memset calls with array initialization (João Barbosa) Pull request description: Follow up to bitcoin#21905 (review). ACKs for top commit: laanwj: re-ACK 1c9255c Crypt-iQ: Code review ACK 1c9255c Tree-SHA512: 4b61dec2094f4781ef1c0427ee3bda3cfea12111274eebc7bc40a84f261d9c1681dd0860c57200bea2456588e44e8e0aecd18545c25f1f1250dd331ab7d05f28

promag force-pushed the 2021-05-msghdr branch from 3c7cb36 to 5d2e40c Compare May 13, 2021 07:45

fanquake added the Refactoring label May 13, 2021

kristapsk approved these changes May 13, 2021

View reviewed changes

jnewbery reviewed May 13, 2021

View reviewed changes

src/protocol.cpp Outdated Show resolved Hide resolved

refactor: Replace memset calls with array initialization

1c9255c

promag force-pushed the 2021-05-msghdr branch from 5d2e40c to 1c9255c Compare May 13, 2021 11:42

laanwj reviewed May 13, 2021

View reviewed changes

laanwj merged commit b34bf2b into bitcoin:master May 13, 2021

promag deleted the 2021-05-msghdr branch May 13, 2021 17:35

barton2526 mentioned this pull request Mar 3, 2022

refactor: Replace memset calls with array initialization gridcoin-community/Gridcoin-Research#2452

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge b34bf2b into merged_master (Bitcoin PR bitcoin/bitcoin#21939)

e8f85e9

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Replace memset calls with array initialization #21939

refactor: Replace memset calls with array initialization #21939

Uh oh!

promag commented May 13, 2021

Uh oh!

laanwj commented May 13, 2021 •

edited

Loading

Uh oh!

promag commented May 13, 2021

Uh oh!

kristapsk left a comment

Uh oh!

Uh oh!

laanwj May 13, 2021

Uh oh!

Crypt-iQ commented May 13, 2021

Uh oh!

maflcko commented May 13, 2021

Uh oh!

laanwj commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

refactor: Replace memset calls with array initialization #21939

refactor: Replace memset calls with array initialization #21939

Uh oh!

Conversation

promag commented May 13, 2021

Uh oh!

laanwj commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 13, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj May 13, 2021

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ commented May 13, 2021

Uh oh!

maflcko commented May 13, 2021

Uh oh!

laanwj commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented May 13, 2021 •

edited

Loading