Fix gcc 9 warnings

laanwj commented at 10:38 am on September 30, 2019: member

Fixes all 3 from #16992 (see commits)

net: Fail instead of truncate command name in CMessageHeader
refactor: Use std::move workaround for unique_ptr upcast only when necessary

laanwj added the label Build system on Sep 30, 2019

laanwj added the label P2P on Sep 30, 2019

laanwj force-pushed on Sep 30, 2019

hebasto commented at 1:17 pm on September 30, 2019: member

Concept ACK.

For “psbt: Add explicit copy operator as default” commit (108f285f76aa5ae803303c329474035c89921f93) there is another solution: 5b2bb0fd3af34463bb382b473cc8b6dee65c0920

laanwj commented at 1:26 pm on September 30, 2019: member

For “psbt: Add explicit copy operator as default” commit (108f285) there is another solution: 5b2bb0f

If all is the same, removing code is strictly better than adding it. If you’re sure that is correct (that that’s exactly what the compiler generates implicitly), I’ll pull in that one.

laanwj force-pushed on Sep 30, 2019

ryanofsky commented at 4:46 pm on September 30, 2019: member

“some compilers” is kind of vague. It was added by @ryanofsky in 79d579f (#14437). Can you elaborate which compilers need this?

Commented #16992 (comment)

hebasto commented at 6:29 pm on September 30, 2019: member

ACK 0515216a9ce55d4faa531c350128b2628391705e, tested on Fedora 30:

 0$ gcc --version | grep gcc
 1gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
 2$ git checkout master 
 3$ make clean && make 2>~/master
 4$ git checkout pr16995
 5$ make clean && make 2>~/pr16995
 6$ diff ~/master ~/pr16995 
 76,23d5
 8< In file included from /usr/include/string.h:494,
 9<                  from ./serialize.h:20,
10<                  from ./netaddress.h:13,
11<                  from ./protocol.h:13,
12<                  from protocol.cpp:6:
13< In function ‘char* strncpy(char*, const char*, size_t)’,
14<     inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:91:12:
15< /usr/include/bits/string_fortified.h:106:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 12 equals destination size [-Wstringop-truncation]
16<   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
17<       |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18< psbt.cpp: In function ‘TransactionError CombinePSBTs(PartiallySignedTransaction&, const std::vector<PartiallySignedTransaction>&)’:
19< psbt.cpp:335:19: warning: implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]
20<   335 |     out = psbtxs[0]; // Copy the first one
21<       |                   ^
22< In file included from psbt.cpp:5:
23< ./psbt.h:398:5: note: because ‘PartiallySignedTransaction’ has user-provided ‘PartiallySignedTransaction::PartiallySignedTransaction(const PartiallySignedTransaction&)’
24<   398 |     PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
25<       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~

DISCLAIMER: My ACK is for 4da0a262e7a2323335d78ae376dde38513eee0e2 commit, not for my own 0515216a9ce55d4faa531c350128b2628391705e.

laanwj force-pushed on Oct 1, 2019

laanwj commented at 12:06 pm on October 1, 2019: member

Pushed a fix for the third warning (inspired by https://cgit.freedesktop.org/libreoffice/core/commit/?id=dc06c8f4989fc28d0c31ebd333e53dfe0e0f5f66).

MarcoFalke commented at 12:23 pm on October 1, 2019: member

A lot of code to fix dumb compiler warnings. I might prefer the warning over the additional code.

laanwj commented at 12:42 pm on October 1, 2019: member

You mean for the std::move one? I’m divided on that one too. I’m not sure how much of a pessimization the extra move is. OTOH if libreoffice deems this important enough to special-case in the build system (after extensive discussion on the LLVM mailing list), maybe it’s somewhat important.

But I think the net message code is an improvement, warning or not. I remember @practicalswift has done a similar thing in the past (to make a static checking tool happy), and it’s been ‘fixed’ a few time it’s time to just bite the bullet there, otherwise you can be sure to get PR after PR for it when GCC9 is in more common use.

ryanofsky commented at 12:46 pm on October 1, 2019: member

I haven’t tested this, but another workaround for the std::move warning might be:

 0--- a/src/interfaces/chain.cpp
 1+++ b/src/interfaces/chain.cpp
 2@@ -242,11 +242,9 @@ class ChainImpl : public Chain
 3 public:
 4     std::unique_ptr<Chain::Lock> lock(bool try_lock) override
 5     {
 6-        auto result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
 7+        std::unique_ptr<Chain::Lock> result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
 8         if (try_lock && result && !*result) return {};
 9-        // std::move necessary on some compilers due to conversion from
10-        // LockImpl to Lock pointer
11-        return std::move(result);
12+        return result;
13     }
14     bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
15     {

This avoids the conversion from LockImpl to Chain::Lock on the return line, moving it up to the result = line, so the return defect is no longer a factor.

laanwj commented at 12:49 pm on October 1, 2019: member

This avoids the conversion from LockImpl to Chain::Lock on the return line, moving it up to the result = line, so the return defect is no longer a factor.

That doesn’t work, !*result is not implemented for Chain::Lock.

ryanofsky commented at 12:59 pm on October 1, 2019: member

That doesn’t work, !*result is not implemented for Chain::Lock.

Oh, I thought it at least compiled, but I missed the error in the build output. The workaround would have to look like:

 0--- a/src/interfaces/chain.cpp
 1+++ b/src/interfaces/chain.cpp
 2@@ -242,11 +242,10 @@ class ChainImpl : public Chain
 3 public:
 4     std::unique_ptr<Chain::Lock> lock(bool try_lock) override
 5     {
 6-        auto result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
 7-        if (try_lock && result && !*result) return {};
 8-        // std::move necessary on some compilers due to conversion from
 9-        // LockImpl to Lock pointer
10-        return std::move(result);
11+        auto lock = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
12+        if (try_lock && lock && !*lock) return {};
13+        std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579
14+        return result;
15     }
16     bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
17     {

laanwj commented at 1:14 pm on October 1, 2019: member

Oh, I thought it at least compiled, but I missed the error in the build output. The workaround would have to look like:

That does seem to avoid the warning.

practicalswift commented at 5:43 am on October 3, 2019: contributor

@ryanofsky @laanwj FWIW, versions prior to Clang 3.9 and GCC 5.1 would try to produce copies. You might want to compile with -Wreturn-std-move-in-c++11 (Clang) to verify that the fix is correct.

Edited for clarity.

laanwj commented at 8:49 am on October 3, 2019: member

I don’t think a Lock is even copyable (it shouldn’t be at least)… and neither should a unique_ptr, by definition

practicalswift commented at 9:27 am on October 3, 2019: contributor

Prior to the resolution of a defect report against ISO C++11, local variable result would have been copied despite being returned by name, due to its not matching the function return type (unique_ptr<Chain::Lock, default_delete<interfaces::Chain::Lock>> vs unique_ptr<interfaces::(anonymous namespace)::LockImpl, default_delete<interfaces::(anonymous namespace)::LockImpl>>).

laanwj commented at 9:33 am on October 3, 2019: member

Ok, I’m getting lost in the C++ issues here closing this. Have to agree a warning is better than a bug.

laanwj closed this on Oct 3, 2019

ryanofsky commented at 1:26 pm on October 3, 2019: member

Prior to the resolution of a defect report against ISO C++11, local variable result would have been copied despite being returned by name, due to its not matching the function return type

There’s no case where result would have been copied because it’s not possible to copy a unique_ptr. Prior to the defect report, returning without std::move would result in a compile error, not a buggy copy, which is why I had to add std::move to avoid the error. Wladimir’s workaround preserves the std::move, so his workaround is also safe and there is no copy on older compilers. My suggested workaround #16995 (comment) preserves the std::move and changes the return statement type so it is safe as well.

practicalswift commented at 1:32 pm on October 3, 2019: contributor

@ryanofsky The text is literally the warning emitted by Clang -Wreturn-std-move-in-c++11 when compiling the code :) Sorry, should have been more clear about that.

0interfaces/chain.cpp:248:16: warning: prior to the resolution of a defect report 
1  against ISO C++11, local variable 'result' would have been copied despite being 
2  returned by name, due to its not matching the function return type 
3  ('unique_ptr<Chain::Lock, default_delete<interfaces::Chain::Lock>>' vs 
4  'unique_ptr<interfaces::(anonymous namespace)::LockImpl, 
5  default_delete<interfaces::(anonymous namespace)::LockImpl>>')
6  [-Wreturn-std-move-in-c++11]

For more information about -Wreturn-std-move-in-c++11, see https://reviews.llvm.org/D43322.

ryanofsky commented at 1:43 pm on October 3, 2019: member

The text is correct if you interpret “would have been copied” to mean “would have been copied if copyable, otherwise result in an error.” It does NOT mean “would have been copied even if not copyable”, which is how Wladimir interpreted your comment what led him to close the PR.

practicalswift commented at 1:44 pm on October 3, 2019: contributor

@ryanofsky Entirely correct :) My only point was that -Wreturn-std-move-in-c++11 might be helpful. Sorry for the ambiguity and distraction.

ryanofsky commented at 1:51 pm on October 3, 2019: member

All right, and in any case this can be tagged up for grabs. The current workaround in the PR which avoids warnings is safe, and so is the other workaround for warnings suggested #16995 (comment). There is no need for any concern that “a warning is better than a bug” if someone using GCC 9 wants to make use of this PR.

laanwj reopened this on Feb 6, 2020

laanwj commented at 12:22 pm on February 6, 2020: member

Changing this to @ryanofsky’s solution

net: Fail instead of truncate command name in CMessageHeader

Replace the memset/strncpy dance in `CMessageHeader::CMessageHeader`
with explicit code that copies then name and asserts the length.

This removes a warning in g++ 9.1.1 and IMO makes the code more readable
by not relying on strncpy padding and silent truncation behavior.

b837b334db

laanwj force-pushed on Feb 6, 2020

refactor: Work around GCC 9 `-Wredundant-move` warning

Use std::move workaround for unique_ptr, for when the C++ compiler lacks
a fix for this issue:
http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579
Do this in a way that avoids a GCC 9 `-Wredundant-move` warning.

ff9c671b11

laanwj force-pushed on Feb 6, 2020

in src/protocol.cpp:93 in b837b334db outdated

90+
91+    // Copy the command name, zero-padding to COMMAND_SIZE bytes
92+    size_t i = 0;
93+    for (; i < COMMAND_SIZE && pszCommand[i] != 0; ++i) pchCommand[i] = pszCommand[i];
94+    assert(pszCommand[i] == 0); // Assert that the command name passed in is not longer than COMMAND_SIZE
95+    for (; i < COMMAND_SIZE; ++i) pchCommand[i] = 0;

ryanofsky commented at 6:31 pm on February 6, 2020:

In commit “net: Fail instead of truncate command name in CMessageHeader” (b837b334db5dd6232725fd2350928ff4fbd3feee)

I don’t want to second guess your code style preferences, but I believe you could just add parentheses around strncpy to silence the warning. IMO, following would be the simplest way of filling the buffer:

0(strncpy(pchCommand, pszCommand, COMMAND_SIZE));
1assert(pchCommand[COMMAND_SIZE-1] == '\0');

Definitely good to drop the memset in either case. That is completely pointless and doesn’t do anything.

sipa commented at 2:21 am on March 19, 2020:

@ryanofsky The memset used to be necessary to zero-pad the copy, no?

laanwj commented at 6:46 pm on March 27, 2020:

I mean—this is pretty trivial code to review, only a few bytes are copied, no high performance platform specific implementation is needed. This also adds an extra (although run-time) assertion that the passed-in string is not truncated (also, pchCommand is not 0-terminated if the length of the command is exactly COMMAND_SIZE). I don’t think we need to rely on any C string functions here. But if you prefer that, sure…

ryanofsky approved

ryanofsky commented at 6:35 pm on February 6, 2020: member

Code review ACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e. Looks good and seems to pass travis, modulo a timeout on one build

hebasto approved

hebasto commented at 6:51 pm on February 6, 2020: member

ACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e, tested on Fedora 31:

 0$ gcc --version | grep gcc
 1gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
 2$ git switch master 
 3$ make clean && make 2>~/master
 4$ git switch pr16995merge-20200206
 5$ make clean && make 2>~/pr16995
 6$ diff ~/master ~/pr16995 
 71,15d0
 8< interfaces/chain.cpp: In member function ‘virtual std::unique_ptr<interfaces::Chain::Lock> interfaces::{anonymous}::ChainImpl::lock(bool)’:
 9< interfaces/chain.cpp:243:25: warning: redundant move in return statement [-Wredundant-move]
10<   243 |         return std::move(result);
11<       |                ~~~~~~~~~^~~~~~~~
12< interfaces/chain.cpp:243:25: note: remove ‘std::move’ call
13< In file included from /usr/include/string.h:495,
14<                  from ./serialize.h:19,
15<                  from ./netaddress.h:13,
16<                  from ./protocol.h:13,
17<                  from protocol.cpp:6:
18< In function ‘char* strncpy(char*, const char*, size_t)’,
19<     inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:89:12:
20< /usr/include/bits/string_fortified.h:106:34: warning: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 12 equals destination size [-Wstringop-truncation]
21<   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
22<       |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

~The only remained warnings are leveldb related.~ UPDATE: no warnings at all if built on top of current master (b5c7665e3083f5daaf2b9f247a59a008f2d689a4).

practicalswift commented at 0:31 am on March 18, 2020: contributor

ACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e – patch looks correct

DrahtBot commented at 3:39 am on March 18, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

sipa commented at 2:23 am on March 19, 2020: member

utACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e

I’ve restarted the s390 travis job that failed due to out of disk space.

MarcoFalke commented at 12:52 pm on March 19, 2020: member

re-run ci

MarcoFalke closed this on Mar 19, 2020

MarcoFalke reopened this on Mar 19, 2020

MarcoFalke commented at 12:54 pm on March 19, 2020: member

The s390x job was removed and I think the only way to tell this to travis is to force push or to close/open. See #15847 (comment)

laanwj merged this on Mar 27, 2020

laanwj closed this on Mar 27, 2020

sidhujag referenced this in commit cb17fbc666 on Mar 28, 2020

MarkLTZ referenced this in commit 3b69b903d9 on Apr 6, 2020

deadalnix referenced this in commit c98781469a on May 20, 2020

deadalnix referenced this in commit 565b950584 on May 21, 2020

ftrader referenced this in commit bda511cc0d on Aug 17, 2020

furszy referenced this in commit 198a014dc6 on Jan 22, 2021

PastaPastaPasta referenced this in commit b902e672b5 on Dec 22, 2021

PastaPastaPasta referenced this in commit 79db0ffaff on Dec 22, 2021

PastaPastaPasta referenced this in commit a94aefd004 on Dec 22, 2021

PastaPastaPasta referenced this in commit 1e30366c40 on Dec 28, 2021

DrahtBot locked this on Feb 15, 2022

Fix gcc 9 warnings #16995

Conflicts