refactor: Replace memset calls with array initialization #21939

promag commented at 7:43 AM on May 13, 2021: member

Follow up to #21905#pullrequestreview-657045699.

promag force-pushed on May 13, 2021

fanquake added the label Refactoring on May 13, 2021

laanwj commented at 8:53 AM on May 13, 2021: member

re-ACK 1c9255c7dd2d4f12bfb508bcc8d123a6354d8842

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

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

</details>

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 commented at 10:14 AM on May 13, 2021: member

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

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

kristapsk approved

kristapsk commented at 10:37 AM on May 13, 2021: contributor

utACK 5d2e40c4d2394758e8026f796de89e8569f8dc86, code looks correct.

refactor: Replace memset calls with array initialization 1c9255c7dd

in src/protocol.cpp:98 in 5d2e40c4d2 outdated

  97 | @@ -105,7 +98,6 @@ CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const
  98 |      for (; i < COMMAND_SIZE; ++i) pchCommand[i] = 0;

jnewbery commented at 11:00 AM on May 13, 2021:

Can this also be removed?

promag commented at 11:05 AM on May 13, 2021:

Right, padding is already zero 👍 good catch

promag commented at 11:42 AM on May 13, 2021:

Removed and updated the comment above. Thanks.

promag force-pushed on May 13, 2021

in src/protocol.cpp:90 in 1c9255c7dd

  92 | -    memset(pchMessageStart, 0, MESSAGE_START_SIZE);
  93 | -    memset(pchCommand, 0, sizeof(pchCommand));
  94 | -    memset(pchChecksum, 0, CHECKSUM_SIZE);
  95 | -}
  96 | -
  97 |  CMessageHeader::CMessageHeader(const MessageStartChars& pchMessageStartIn, const char* pszCommand, unsigned int nMessageSizeIn)

laanwj commented at 2:42 PM on May 13, 2021:

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 commented at 2:47 PM on May 13, 2021: contributor

Code review ACK 1c9255c7dd2d4f12bfb508bcc8d123a6354d8842

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?

MarcoFalke commented at 4:03 PM on May 13, 2021: member

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

laanwj commented at 4:54 PM on May 13, 2021: member

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.

laanwj merged this on May 13, 2021

laanwj closed this on May 13, 2021

promag deleted the branch on May 13, 2021

sidhujag referenced this in commit 3b43f88056 on May 13, 2021

gwillen referenced this in commit e8f85e9062 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022