Fix gcc 9 warnings #16995

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

Concept ACK.

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

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.

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

Commented #16992 (comment)

ACK 0515216a9ce55d4faa531c350128b2628391705e, tested on Fedora 30:

$ gcc --version | grep gcc
gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
$ git checkout master 
$ make clean && make 2>~/master
$ git checkout pr16995
$ make clean && make 2>~/pr16995
$ diff ~/master ~/pr16995 
6,23d5
< In file included from /usr/include/string.h:494,
<                  from ./serialize.h:20,
<                  from ./netaddress.h:13,
<                  from ./protocol.h:13,
<                  from protocol.cpp:6:
< In function ‘char* strncpy(char*, const char*, size_t)’,
<     inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:91:12:
< /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]
<   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
<       |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
< psbt.cpp: In function ‘TransactionError CombinePSBTs(PartiallySignedTransaction&, const std::vector<PartiallySignedTransaction>&)’:
< psbt.cpp:335:19: warning: implicitly-declared ‘PartiallySignedTransaction& PartiallySignedTransaction::operator=(const PartiallySignedTransaction&)’ is deprecated [-Wdeprecated-copy]
<   335 |     out = psbtxs[0]; // Copy the first one
<       |                   ^
< In file included from psbt.cpp:5:
< ./psbt.h:398:5: note: because ‘PartiallySignedTransaction’ has user-provided ‘PartiallySignedTransaction::PartiallySignedTransaction(const PartiallySignedTransaction&)’
<   398 |     PartiallySignedTransaction(const PartiallySignedTransaction& psbt_in) : tx(psbt_in.tx), inputs(psbt_in.inputs), outputs(psbt_in.outputs), unknown(psbt_in.unknown) {}
<       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~

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

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

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

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.

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

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

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.

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.

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:

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

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.

@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.

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

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>>).

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

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.

@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.

interfaces/chain.cpp:248:16: warning: 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>>')
  [-Wreturn-std-move-in-c++11]

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

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.

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

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.

Changing this to @ryanofsky's solution

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

ACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e, tested on Fedora 31:

$ gcc --version | grep gcc
gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1)
$ git switch master 
$ make clean && make 2>~/master
$ git switch pr16995merge-20200206
$ make clean && make 2>~/pr16995
$ diff ~/master ~/pr16995 
1,15d0
< interfaces/chain.cpp: In member function ‘virtual std::unique_ptr<interfaces::Chain::Lock> interfaces::{anonymous}::ChainImpl::lock(bool)’:
< interfaces/chain.cpp:243:25: warning: redundant move in return statement [-Wredundant-move]
<   243 |         return std::move(result);
<       |                ~~~~~~~~~^~~~~~~~
< interfaces/chain.cpp:243:25: note: remove ‘std::move’ call
< In file included from /usr/include/string.h:495,
<                  from ./serialize.h:19,
<                  from ./netaddress.h:13,
<                  from ./protocol.h:13,
<                  from protocol.cpp:6:
< In function ‘char* strncpy(char*, const char*, size_t)’,
<     inlined from ‘CMessageHeader::CMessageHeader(const unsigned char (&)[4], const char*, unsigned int)’ at protocol.cpp:89:12:
< /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]
<   106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
<       |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

ACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e -- patch looks correct

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

utACK ff9c671b11d40e5d0623eff3dd12e48cbaafb34e

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

re-run ci

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)