Fix gcc 9 warnings #16995

laanwj · 2019-09-30T10:38:32Z

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

hebasto · 2019-09-30T13:17:40Z

Concept ACK.

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

laanwj · 2019-09-30T13:26:50Z

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.

ryanofsky · 2019-09-30T16:46:18Z

"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 · 2019-09-30T18:29:50Z

ACK 0515216, 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 4da0a26 commit, not for my own 0515216.

laanwj · 2019-10-01T12:06:22Z

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

maflcko · 2019-10-01T12:23:21Z

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

laanwj · 2019-10-01T12:42:11Z

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 · 2019-10-01T12:46:12Z

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.

laanwj · 2019-10-01T12:49:55Z

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 · 2019-10-01T12:59:33Z

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
     {

laanwj · 2019-10-01T13:14:46Z

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 · 2019-10-03T05:43:50Z

@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 · 2019-10-03T08:49:25Z

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

practicalswift · 2019-10-03T09:27:25Z

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 · 2019-10-03T09:33:23Z

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

ryanofsky · 2019-10-03T13:26:30Z

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 · 2019-10-03T13:32:45Z

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

ryanofsky · 2019-10-03T13:43:28Z

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 · 2019-10-03T13:44:11Z

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

ryanofsky · 2019-10-03T13:51:24Z

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 · 2020-02-06T12:22:25Z

Changing this to @ryanofsky's solution

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.

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.

ryanofsky

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

ryanofsky · 2020-02-06T18:31:30Z

src/protocol.cpp

+    size_t i = 0;
+    for (; i < COMMAND_SIZE && pszCommand[i] != 0; ++i) pchCommand[i] = pszCommand[i];
+    assert(pszCommand[i] == 0); // Assert that the command name passed in is not longer than COMMAND_SIZE
+    for (; i < COMMAND_SIZE; ++i) pchCommand[i] = 0;


In commit "net: Fail instead of truncate command name in CMessageHeader" (b837b33)

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:

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

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

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

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…

hebasto

ACK ff9c671, 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 (b5c7665).

practicalswift · 2020-03-18T00:31:38Z

ACK ff9c671 -- patch looks correct

DrahtBot · 2020-03-18T03:39:12Z

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

Conflicts

No conflicts as of last run.

sipa · 2020-03-19T02:23:27Z

utACK ff9c671

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

maflcko · 2020-03-19T12:52:59Z

re-run ci

maflcko · 2020-03-19T12:54:31Z

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)

ff9c671 refactor: Work around GCC 9 `-Wredundant-move` warning (Russell Yanofsky) b837b33 net: Fail instead of truncate command name in CMessageHeader (Wladimir J. van der Laan) Pull request description: Fixes all 3 from bitcoin#16992 (see commits) - net: Fail instead of truncate command name in CMessageHeader - refactor: Use std::move workaround for unique_ptr upcast only when necessary ACKs for top commit: practicalswift: ACK ff9c671 -- patch looks correct sipa: utACK ff9c671 ryanofsky: Code review ACK ff9c671. Looks good and seems to pass travis, modulo a timeout on one build hebasto: ACK ff9c671, tested on Fedora 31: Tree-SHA512: 52d8c13aaf0d56f9bc546a98d7f853eae21f7e325b202fdeb2286b19a9a0ee308634c644b039f60ad8043421e382381cbf1bce58d9f807547f928621c7d245d0

- Fix gcc 9 warnings bitcoin#16995 - guix: Remove now-unnecessary gcc make flag bitcoin#18320 - build: Remove workaround for ancient libtool bitcoin#17066 - build: Add variable printing target to Makefiles bitcoin#17087 - Disable _FORTIFY_SOURCE when enable-debug bitcoin#17033

…geHeader Summary: 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. bitcoin/bitcoin@b837b33 --- This is a partial backport of Core [[bitcoin/bitcoin#16995 | PR16995]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6164

Summary: 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. bitcoin/bitcoin@ff9c671 --- This completes the backport of Core [[bitcoin/bitcoin#16995 | PR16995]] Test Plan: ninja check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6165

…geHeader Summary: 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. bitcoin/bitcoin@b837b33 --- This is a partial backport of Core [[bitcoin/bitcoin#16995 | PR16995]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6164

…eader c223041 net: Fail instead of truncate command name in CMessageHeader (Wladimir J. van der Laan) Pull request description: Fix the `-Wstringop-truncation` warning reported in #2076, cherry-picking the first commit of bitcoin#16995 > 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. ACKs for top commit: furszy: utACK c223041 Fuzzbawls: utACK c223041 Tree-SHA512: 1c1fecc990f4d19fa5a1eb559132e81ee904af5557e3a63256d08741e201996de1f116267025317cf0c236872d121af958798a4de9bbc9a5afbcd601b768a9aa

ff9c671 refactor: Work around GCC 9 `-Wredundant-move` warning (Russell Yanofsky) b837b33 net: Fail instead of truncate command name in CMessageHeader (Wladimir J. van der Laan) Pull request description: Fixes all 3 from bitcoin#16992 (see commits) - net: Fail instead of truncate command name in CMessageHeader - refactor: Use std::move workaround for unique_ptr upcast only when necessary ACKs for top commit: practicalswift: ACK ff9c671 -- patch looks correct sipa: utACK ff9c671 ryanofsky: Code review ACK ff9c671. Looks good and seems to pass travis, modulo a timeout on one build hebasto: ACK ff9c671, tested on Fedora 31: Tree-SHA512: 52d8c13aaf0d56f9bc546a98d7f853eae21f7e325b202fdeb2286b19a9a0ee308634c644b039f60ad8043421e382381cbf1bce58d9f807547f928621c7d245d0

laanwj added Build system P2P labels Sep 30, 2019

laanwj force-pushed the 2019_09_resolve_gcc_warnings branch from 6810868 to 108f285 Compare September 30, 2019 10:57

laanwj force-pushed the 2019_09_resolve_gcc_warnings branch from 108f285 to 0515216 Compare September 30, 2019 13:37

laanwj force-pushed the 2019_09_resolve_gcc_warnings branch from 4e5bc74 to 0515216 Compare October 1, 2019 11:33

laanwj closed this Oct 3, 2019

laanwj mentioned this pull request Oct 3, 2019

Compilation warnings with g++ 9.1.1 #16992

Closed

laanwj mentioned this pull request Nov 2, 2019

Remove redundant copy constructors #17349

Merged

hebasto mentioned this pull request Feb 2, 2020

Silence "redundant move in return statement" warning on GCC9 #18053

Closed

laanwj reopened this Feb 6, 2020

laanwj force-pushed the 2019_09_resolve_gcc_warnings branch from 92bd4b9 to 6a942f2 Compare February 6, 2020 12:41

refactor: Work around GCC 9 -Wredundant-move warning

ff9c671

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.

laanwj force-pushed the 2019_09_resolve_gcc_warnings branch from 6a942f2 to ff9c671 Compare February 6, 2020 12:43

ryanofsky approved these changes Feb 6, 2020

View reviewed changes

hebasto approved these changes Feb 6, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 18, 2020

Avoid strncpy for CMessageHeader::pchCommand #18367

Closed

maflcko closed this Mar 19, 2020

maflcko reopened this Mar 19, 2020

laanwj merged commit f2c416b into bitcoin:master Mar 27, 2020

maflcko mentioned this pull request Mar 28, 2020

Add fuzzing harnesses #18431

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

random-zebra mentioned this pull request Dec 20, 2020

[Net] Fail instead of truncate command name in CMessageHeader PIVX-Project/PIVX#2090

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Fix gcc 9 warnings #16995

Fix gcc 9 warnings #16995

Uh oh!

Conversation

laanwj commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Sep 30, 2019

Uh oh!

laanwj commented Sep 30, 2019

Uh oh!

ryanofsky commented Sep 30, 2019

Uh oh!

hebasto commented Sep 30, 2019

Uh oh!

laanwj commented Oct 1, 2019

Uh oh!

maflcko commented Oct 1, 2019

Uh oh!

laanwj commented Oct 1, 2019

Uh oh!

ryanofsky commented Oct 1, 2019

Uh oh!

laanwj commented Oct 1, 2019

Uh oh!

ryanofsky commented Oct 1, 2019

Uh oh!

laanwj commented Oct 1, 2019

Uh oh!

practicalswift commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 3, 2019

Uh oh!

laanwj commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 3, 2019

Uh oh!

practicalswift commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 3, 2019

Uh oh!

practicalswift commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Oct 3, 2019

Uh oh!

laanwj commented Feb 6, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

sipa Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Mar 18, 2020

Uh oh!

DrahtBot commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Mar 19, 2020

Uh oh!

maflcko commented Mar 19, 2020

laanwj commented Sep 30, 2019 •

edited

Loading

practicalswift commented Oct 3, 2019 •

edited

Loading

laanwj commented Oct 3, 2019 •

edited

Loading

laanwj commented Oct 3, 2019 •

edited

Loading

practicalswift commented Oct 3, 2019 •

edited

Loading

practicalswift commented Oct 3, 2019 •

edited

Loading

laanwj Mar 27, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading

DrahtBot commented Mar 18, 2020 •

edited

Loading