build: Disable deprecated-copy warning only when external warnings are enabled

MarcoFalke commented at 11:05 am on June 16, 2021: member

Fixes #18967

Alternative to https://github.com/bitcoin/bitcoin/pull/22240

MarcoFalke commented at 11:57 am on June 16, 2021: member

Tested warnings-free on Focal with:

clang (./configure --with-incompatible-bdb --enable-suppress-external-warnings CC=clang CXX=clang++ && make clean && make -j $(nproc))
gcc (./configure --with-incompatible-bdb --enable-suppress-external-warnings && make clean && make -j $(nproc))
depends (CONFIG_SITE="$PWD/depends/x86_64-pc-linux-gnu/share/config.site" ./configure --enable-suppress-external-warnings && make clean && make -j $(nproc))

Also tested on Fedora 34 with clang and gcc.

MarcoFalke commented at 11:59 am on June 16, 2021: member

@fanquake (or someone else) Can you test this one please: #18738 (comment)

practicalswift commented at 12:00 pm on June 16, 2021: contributor

Concept ACK

in configure.ac:479 in fa9824ca11 outdated

474@@ -475,7 +475,9 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
475   AX_CHECK_COMPILE_FLAG([-Wself-assign],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-self-assign"],,[[$CXXFLAG_WERROR]])
476   AX_CHECK_COMPILE_FLAG([-Wunused-local-typedef],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-unused-local-typedef"],,[[$CXXFLAG_WERROR]])
477   AX_CHECK_COMPILE_FLAG([-Wimplicit-fallthrough],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-implicit-fallthrough"],,[[$CXXFLAG_WERROR]])
478-  AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])
479+  if test x$suppress_external_warnings == xno ; then
480+    AX_CHECK_COMPILE_FLAG([-Wdeprecated-copy],[NOWARN_CXXFLAGS="$NOWARN_CXXFLAGS -Wno-deprecated-copy"],,[[$CXXFLAG_WERROR]])

fanquake commented at 12:11 pm on June 16, 2021:

I’m almost certain @hebasto already made this comment, but I must have imagined it. Can you move this up above AX_CHECK_COMPILE_FLAG([-Wdocumentation], inside the existing if test x$suppress_external_warnings != xno ; then check. Then this is ok.

MarcoFalke commented at 12:27 pm on June 16, 2021:

inside the existing if test x$suppress_external_warnings != xno ; then check.

That wouldn’t work. The check needs to be == xno, not != xno. Alternatively it can be != xyes, I guess.

Also, I want to keep the NOWARN_CXXFLAGS in one section and not mix it with the WARN_CXXFLAGS section

fanquake commented at 12:43 pm on June 16, 2021:

Right, in this case we’re removing a suppression only when enabling other suppressions. Changing this to !=yes would better match how we generally do this in configure

MarcoFalke commented at 1:45 pm on June 16, 2021:

Thanks, pushed a fix. Didn’t re-test.

0-  if test x$suppress_external_warnings == xno ; then
1+  if test x$suppress_external_warnings != xyes ; then

DrahtBot added the label Build system on Jun 16, 2021

build: Disable deprecated-copy warning only when external warnings are enabled 1111457d74

MarcoFalke force-pushed on Jun 16, 2021

DrahtBot commented at 3:14 pm on June 16, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21430 (build: Add -Werror=implicit-fallthrough compile flag by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Subawit approved

fanquake approved

fanquake commented at 3:43 am on June 17, 2021: member

tACK 1111457d7433c2cca1d7e98946f6954e3a0280ef

fanquake merged this on Jun 17, 2021

fanquake closed this on Jun 17, 2021

MarcoFalke deleted the branch on Jun 17, 2021

sidhujag referenced this in commit f5c50bd5c7 on Jun 18, 2021

vasild commented at 3:29 pm on June 21, 2021: member

According to Clang 13.0.0 we have deprecated-copy warnings in our code too:

 0clang++-devel -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config  -DDEBUG -DDEBUG_LOCKORDER -DABORT_ON_FAILED_ASSUME  -I. -I./secp256k1/include -isystem /usr/local/include/db5 -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -isystem /usr/local/include -I./leveldb/include -I./leveldb/helpers/memenv -I./univalue/include   -isystem /usr/local/include -I/usr/local/include -D_THREAD_SAFE -pthread  -I/usr/local/include -Wno-deprecated-declarations -Werror -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DPROVIDE_FUZZ_MAIN_FUNCTION -O0 -g3 -ftrapv -Wstack-protector -fstack-protector-all -fcf-protection=full -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wswitch -Wthread-safety -Wrange-loop-analysis -Wredundant-decls -Wunused-variable -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Wsign-compare -Woverloaded-virtual -Wsuggest-override -Wunreachable-code-loop-increment -Wdocumentation -Wno-unused-parameter -Wno-self-assign -Wno-unused-local-typedef -Wno-implicit-fallthrough    -fPIE  -MT policy/libbitcoin_server_a-rbf.o -MD -MP -MF policy/.deps/libbitcoin_server_a-rbf.Tpo -c -o policy/libbitcoin_server_a-rbf.o `test -f 'policy/rbf.cpp' || echo './'`policy/rbf.cpp
 1
 2In file included from policy/rbf.cpp:5:
 3In file included from ./policy/rbf.h:8:
 4In file included from ./txmempool.h:24:
 5./util/epochguard.h:53:17: error: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
 6        Marker& operator=(const Marker&) = delete;
 7                ^
 8./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
 9class CTxMemPoolEntry
10      ^
11policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
12    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
13                            ^
141 error generated.
15
16
17
18
19In file included from test/fuzz/policy_estimator.cpp:9:
20In file included from ./test/fuzz/util.h:27:
21In file included from ./txmempool.h:24:
22./util/epochguard.h:53:17: error: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]
23        Marker& operator=(const Marker&) = delete;
24                ^
25./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
26class CTxMemPoolEntry
27      ^
28/usr/include/c++/v1/memory:1876:31: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
29            ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
30                              ^
31/usr/include/c++/v1/memory:1768:18: note: in instantiation of function template specialization 'std::allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
32            {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}
33                 ^
34/usr/include/c++/v1/memory:1595:14: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::__construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
35            {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),
36             ^
37/usr/include/c++/v1/vector:924:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
38    __alloc_traits::construct(this->__alloc(), _VSTD::__to_address(__tx.__pos_),
39                    ^
40/usr/include/c++/v1/vector:1652:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::__construct_one_at_end<CTxMemPoolEntry>' requested here
41        __construct_one_at_end(_VSTD::move(__x));
42        ^
43test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
44                    mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
45                                    ^
461 error generated.

cc @ajtowns the below fixes it:

 0diff --git i/src/policy/rbf.cpp w/src/policy/rbf.cpp
 1index 8125b41c41..10859d256d 100644
 2--- i/src/policy/rbf.cpp
 3+++ w/src/policy/rbf.cpp
 4@@ -23,13 +23,13 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
 5     }
 6 
 7     // If all the inputs have nSequence >= maxint-1, it still might be
 8     // signaled for RBF if any unconfirmed parents have signaled.
 9     uint64_t noLimit = std::numeric_limits<uint64_t>::max();
10     std::string dummy;
11-    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());
12+    const CTxMemPoolEntry& entry = *pool.mapTx.find(tx.GetHash());
13     pool.CalculateMemPoolAncestors(entry, setAncestors, noLimit, noLimit, noLimit, noLimit, dummy, false);
14 
15     for (CTxMemPool::txiter it : setAncestors) {
16         if (SignalsOptInRBF(it->GetTx())) {
17             return RBFTransactionState::REPLACEABLE_BIP125;
18         }
19diff --git i/src/util/epochguard.h w/src/util/epochguard.h
20index 1570ec4eb4..79c0abb4eb 100644
21--- i/src/util/epochguard.h
22+++ w/src/util/epochguard.h
23@@ -42,12 +42,16 @@ public:
24     Epoch& operator=(const Epoch&) = delete;
25 
26     bool guarded() const { return m_guarded; }
27 
28     class Marker
29     {
30+    public:
31+        Marker() = default;
32+        Marker(const Marker& other) = default;
33+
34     private:
35         uint64_t m_marker = 0;
36 
37         // only allow modification via Epoch member functions
38         friend class Epoch;
39         Marker& operator=(const Marker&) = delete;

PastaPastaPasta referenced this in commit 65d37d9565 on Jun 27, 2021

PastaPastaPasta referenced this in commit 9266377560 on Jun 28, 2021

PastaPastaPasta referenced this in commit f409a32467 on Jun 29, 2021

PastaPastaPasta referenced this in commit 9c480d085e on Jul 1, 2021

PastaPastaPasta referenced this in commit bfb552a06b on Jul 1, 2021

PastaPastaPasta referenced this in commit 6b12d652bf on Sep 21, 2021

gwillen referenced this in commit 98800c69d2 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

build: Disable deprecated-copy warning only when external warnings are enabled #22258

Conflicts