build: Fix a few compilation issues with Clang 7 and -Werror #12678

vasild commented at 7:02 PM on March 12, 2018: member

No description provided.

practicalswift commented at 7:11 PM on March 12, 2018: contributor

Concept ACK

fanquake assigned theuni on Mar 13, 2018

fanquake commented at 2:25 AM on March 13, 2018: member

@vasild Have you submitted any of these boost m4 changes upstream?

fanquake added the label Build system on Mar 13, 2018

theuni commented at 4:39 AM on March 13, 2018: member

I'd really rather not go down this path. We can't expect to find only warning-free headers, especially with system libs that we can't control.

Have you tried using --enable-werror rather than adding directly to *FLAGS? That option is there to solve exactly this issue, as well as to whitelist no-false-positive warnings. We could probably stand to be more aggressive with those.

practicalswift commented at 6:59 AM on March 13, 2018: contributor

@theuni But the net_processing.h change is OK?

vasild commented at 8:20 AM on March 13, 2018: member

@fanquake

Have you submitted any of these boost m4 changes upstream?

No. I noticed these files are imported from another project (boost), but saw that there are already some local mods, so decided that it may be ok to change them.

Anyway, I should submit these to boost.

vasild commented at 9:05 AM on March 13, 2018: member

@theuni

We can't expect to find only warning-free headers, especially with system libs that we can't control.

Yes, that is true. But notice that in this case the warnings do not come from system headers or from headers that are outside of our control. They come from files within the bitcoin codebase: configure.ac, build-aux/m4/ax_boost_chrono.m4 and build-aux/m4/ax_boost_unit_test_framework.m4.

If we are more relaxed during configure-time checks (e.g. no -Werror), then there is a chance that we pronounce a library which produces warnings as "available" and later if we build with stricter flags (e.g. -Werror), then the build itself would choke when it tries to use that library.

Have you tried using --enable-werror rather than adding directly to *FLAGS? That option is there to solve exactly this issue, as well as to whitelist no-false-positive warnings.

Yes, it only uses the stricter flags when compiling files from src/ and not during the configure checks. I was upset that it only adds -Werror=vla -Werror=thread-safety-analysis.

We could probably stand to be more aggressive with those.

Yes. I would suggest to use just -Werror instead of -Werror=vla -Werror=thread-safety-analysis. That could make it fail in environments where it previously succeeded. I guess it would need more extensive testing - at least with all supported compilers, right?

Should I adjust this pull request to do that - use just -Werror?

Regardless of that, I think the changes to configure.ac are good and can/should go in. The changes to build-aux/ have the issue of modifying imported source from boost and the risk of those changes being wiped away in case of re-import of a newer version (but there is a project to ditch boost altogether).

How to proceed?

in src/net_processing.h:46 in 9c31753d62 outdated

  41 | @@ -42,6 +42,8 @@ class PeerLogicValidation : public CValidationInterface, public NetEventsInterfa
  42 |  public:
  43 |      explicit PeerLogicValidation(CConnman* connman, CScheduler &scheduler);
  44 |  
  45 | +    virtual ~PeerLogicValidation() = default;
  46 | +

laanwj commented at 10:04 AM on March 13, 2018:

ACK, also seen this warning.

laanwj commented at 10:06 AM on March 13, 2018: member

Defer to @theuni on the build system changes, but adding the virtual destructor when inheriting from a class with virtual methods is good practice.

vasild commented at 11:17 AM on March 13, 2018: member

@fanquake

Have you submitted any of these boost m4 changes upstream?

There you go: https://github.com/peti/autoconf-archive/pull/148

practicalswift commented at 4:15 PM on March 13, 2018: contributor

What about submitting the uncontroversial PeerLogicValidation fix as a separate PR? I've seen that warning myself and it would be nice to get rid of it :-) The m4 changes seem to require more discussion.

vasild force-pushed on Mar 13, 2018

vasild commented at 8:07 PM on March 13, 2018: member

@practicalswift

What about submitting the uncontroversial PeerLogicValidation fix as a separate PR? I've seen that warning myself and it would be nice to get rid of it :-)

there you go: #12680

The m4 changes seem to require more discussion.

Yes. The m4 boost changes have been taken upstream, so I have changed this pull request to apply/take all changes from upstream to those two files. I guess those should now be ok, because it just updates from upstream, @theuni?

If the two changes in configure.ac are undesirable (MSG_* and libminiupnpc check) I could ditch them away from this pull request.

Unrelated to the outcome of the above, I will consider changing --enable-werror to use just -Werror instead of -Werror=vla -Werror=thread-safety-analysis.

Cheers!

laanwj renamed this:
~~Scripts and tools: Fix a few compilation issues with Clang 7 and -Werror~~
build: Fix a few compilation issues with Clang 7 and -Werror
on Mar 14, 2018

in configure.ac:758 in af01308678 outdated

 770 | @@ -771,7 +771,7 @@ dnl Check for libminiupnpc (optional)
 771 |  if test x$use_upnp != xno; then
 772 |    AC_CHECK_HEADERS(
 773 |      [miniupnpc/miniwget.h miniupnpc/miniupnpc.h miniupnpc/upnpcommands.h miniupnpc/upnperrors.h],
 774 | -    [AC_CHECK_LIB([miniupnpc], [main],[MINIUPNPC_LIBS=-lminiupnpc], [have_miniupnpc=no])],
 775 | +    [AC_CHECK_LIB([miniupnpc], [upnpDiscover], [MINIUPNPC_LIBS=-lminiupnpc], [have_miniupnpc=no])],

eklitzke commented at 6:38 AM on March 15, 2018:

I don't see how this is related to Clang 7, this is checking for the visibility of a symbol. This might be needed on your version of upnp but if so that should be another PR in my opinion.

vasild commented at 8:40 AM on March 15, 2018:

The commit message of that change reads:

    Do not check for main() in libminiupnpc
    
    main() { main(); } causes "infinite recursion" compilation warning
    which with -Werror fails the check.

Maybe that is not clear enough, let me elaborate: AC_CHECK_LIB([lib], [func], ...) generates a test program like main() { func(); ... } and tries to link it with -llib. The idea being that if func() is not present in lib it will fail to link with an undefined reference error. So the current usage leads to a test program like main() { main(); ... } which generates a legit compiler warning of "infinite recursion". At least Clang 7 emits that warning, maybe other compilers do so too.

Anyway, I think checking for function main() in libminiupnpc is a misuse of AC_CHECK_LIB because main() does not exist in libminiupnpc and it passes because it relies on how the check is performed internally. If AC_CHECK_LIB starts using another method of checking if a function is present in a library (e.g. nm(1)) then the check may as well fail.

eklitzke commented at 1:16 PM on March 15, 2018:

That makes sense, thanks for clarifying.

in configure.ac:649 in af01308678 outdated

 645 | @@ -646,15 +646,15 @@ AC_CHECK_DECLS([__builtin_clz, __builtin_clzl, __builtin_clzll])
 646 |  dnl Check for MSG_NOSIGNAL
 647 |  AC_MSG_CHECKING(for MSG_NOSIGNAL)
 648 |  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/socket.h>]],
 649 | - [[ int f = MSG_NOSIGNAL; ]])],
 650 | + [[ return MSG_NOSIGNAL; ]])],

eklitzke commented at 6:40 AM on March 15, 2018:

This seems broken to me in the first place, isn't MSG_NOSIGNAL a preprocessor define? If so there's no need to check for it in configure.ac, we can just use #if defined() in net.cpp and netbase.cpp.

vasild commented at 8:54 AM on March 15, 2018:

MSG_NOSIGNAL is a macro in my environment and maybe in lots of other environments too. So this check can be wiped away from configure.ac and #ifdef MSG_NOSIGNAL be used instead of #ifdef HAVE_MSG_NOSIGNAL in the source code. On my environment that will work.

I guess the reason for this check in configure.ac is that we are not guaranteed that it will be a macro. Maybe somewhere it could be const int MSG_NOSIGNAL = ...?

My change intends to fix an "unused variable" warning, with a minimum chance of breaking something else.

eklitzke commented at 1:15 PM on March 15, 2018:

I believe all of these options are required to be macros by the standard (precisely for reasons like this). Also note that const int FOO = 1 and #define FOO 1 are not the same, using a const value is strictly worse as it requires declaring an extern value that gets storage space allocated in the program. So there's no reason an implementation would ever do things that way.

The problem with keeping logic like this "just in case" is that these AC_COMPILE_IFELSE checks are relatively expensive, so we should only be using them for features that actually require compilation.

vasild commented at 7:08 PM on March 15, 2018:

You are right - const int requires storage somewhere. I have removed the checks from configure.ac and updated the pull request.

eklitzke changes_requested

ax_boost_{chrono,unit_test_framework}.m4: take changes from upstream

Apply changes to
build-aux/m4/ax_boost_chrono.m4 and
build-aux/m4/ax_boost_unit_test_framework.m4
from upstream: https://github.com/peti/autoconf-archive

8c632f73c2

Do not check for main() in libminiupnpc

main() { main(); } causes "infinite recursion" compilation warning
which with -Werror fails the check.

71129e0265

Remove redundant checks for MSG_* from configure.ac

It is redundant to check for the presence of MSG_NOSIGNAL macro in
configure.ac, define HAVE_MSG_NOSIGNAL and then check whether the later
is defined in the source code. Instead we can check directly whether
MSG_NOSIGNAL is defined. Same for MSG_DONTWAIT.

In addition to that, the checks we had in configure.ac produce a
compiler warning about unused variable and thus could fail if
-Werror is present and erroneously proclaim that the macros are
not available.

8ae413235d

vasild force-pushed on Mar 15, 2018

theuni approved

theuni commented at 7:46 PM on March 15, 2018: member

utACK 8ae413235d4b3f68f0858a3ce2cd65291f9cbe64. Thanks!

eklitzke commented at 7:26 AM on March 19, 2018: contributor

utACK 8ae413235d4b3f68f0858a3ce2cd65291f9cbe64

laanwj merged this on Mar 19, 2018

laanwj closed this on Mar 19, 2018

laanwj referenced this in commit 6324c68aa0 on Mar 19, 2018

vasild deleted the branch on Mar 19, 2018

MarcoFalke referenced this in commit 415f2bff69 on Jul 26, 2018

PastaPastaPasta referenced this in commit e1e303b179 on Jul 29, 2020

PastaPastaPasta referenced this in commit 451d36fc45 on Dec 12, 2020

PastaPastaPasta referenced this in commit 04d0ebc960 on Dec 12, 2020

PastaPastaPasta referenced this in commit bb3de8a77d on Dec 15, 2020

PastaPastaPasta referenced this in commit 7112bea863 on Dec 15, 2020

PastaPastaPasta referenced this in commit a09555c389 on Dec 15, 2020

PastaPastaPasta referenced this in commit 455f92d65b on Dec 18, 2020

gades referenced this in commit e011d05a60 on Jun 25, 2021

gades referenced this in commit 6e4dc7c4b5 on Jun 28, 2021

MarcoFalke locked this on Sep 8, 2021