build: Add -Werror=implicit-fallthrough compile flag #21430

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:210313-fall changing 7 files +14 −7
  1. hebasto commented at 9:10 pm on March 13, 2021: member
  2. DrahtBot added the label Build system on Mar 13, 2021
  3. DrahtBot added the label GUI on Mar 13, 2021
  4. DrahtBot added the label Mining on Mar 13, 2021
  5. DrahtBot added the label RPC/REST/ZMQ on Mar 13, 2021
  6. DrahtBot commented at 11:47 pm on March 13, 2021: member

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

    Conflicts

    No conflicts as of last run.

  7. MarcoFalke removed the label GUI on Mar 14, 2021
  8. MarcoFalke removed the label Mining on Mar 14, 2021
  9. MarcoFalke removed the label RPC/REST/ZMQ on Mar 14, 2021
  10. practicalswift commented at 4:28 pm on March 15, 2021: contributor

    cr ACK e95d14e07e9215d6d8ba1a56d13485a18e3550ba: patch looks correct!

    I’ve always been worried about introduction of bugs in our code due to unintentional fallthroughs. Thanks for reducing the number of things to worry about by one :)

    As always: explicit is better than implicit! :)

  11. in configure.ac:443 in e95d14e07e outdated 
    438@@ -439,6 +439,7 @@ if test "x$enable_werror" = "xyes"; then
439                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
440   AX_CHECK_COMPILE_FLAG([-Werror=unreachable-code-loop-increment],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
441   AX_CHECK_COMPILE_FLAG([-Werror=mismatched-tags], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=mismatched-tags"], [], [$CXXFLAG_WERROR])
442+  AX_CHECK_COMPILE_FLAG([-Werror=implicit-fallthrough], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=implicit-fallthrough"], [], [$CXXFLAG_WERROR])
443 fi
444 
445 if test "x$CXXFLAGS_overridden" = "xno"; then
    MarcoFalke commented at 4:34 pm on March 15, 2021:
    any reason to not enable the warning as well?
    hebasto commented at 4:47 pm on March 15, 2021:

    You are right!

    From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:

    -Wimplicit-fallthrough is the same as -Wimplicit-fallthrough=3…

    The -Wimplicit-fallthrough=3 warning is enabled by -Wextra.

    But that is not the case for clang. Going to fix it now.

    hebasto commented at 5:02 pm on March 15, 2021:
    Thanks! Updated.
  12. MarcoFalke approved
  13. MarcoFalke commented at 4:35 pm on March 15, 2021: member

    cr ACK e95d14e07e9215d6d8ba1a56d13485a18e3550ba

    didn’t review the leveldb build change

  14. MarcoFalke added the label Needs Guix build on Mar 15, 2021
  15. hebasto force-pushed on Mar 15, 2021
  16. hebasto commented at 5:02 pm on March 15, 2021: member

    Updated e95d14e07e9215d6d8ba1a56d13485a18e3550ba -> 2f3436bcad878a313ce53bab2fc8a2fed9056351 (pr21430.01 -> pr21430.02, diff):

    any reason to not enable the warning as well? @MarcoFalke didn’t review the leveldb build change

    https://github.com/bitcoin/bitcoin/blob/c771fc0dc1bcb48fc6aa77b61c1ff31742ac8bb7/src/leveldb/util/hash.cc#L11-L12

  17. practicalswift commented at 5:46 pm on March 15, 2021: contributor

    cr re-ACK 2f3436bcad878a313ce53bab2fc8a2fed9056351

    Consider submitting the tinyformat.h changes upstream too (https://github.com/c42f/tinyformat).

  18. DrahtBot removed the label Needs Guix build on Mar 19, 2021
  19. MarcoFalke deleted a comment on Apr 6, 2021
  20. DrahtBot added the label Needs rebase on Apr 6, 2021
  21. hebasto force-pushed on Apr 13, 2021
  22. hebasto commented at 7:10 am on April 13, 2021: member
    Rebased 2f3436bcad878a313ce53bab2fc8a2fed9056351 -> 5bc16d28e73a068382e66fce9ffcb604a3db34dc (pr21430.02 -> pr21430.03) due to the conflicts with #21610 and #21613.
  23. DrahtBot removed the label Needs rebase on Apr 13, 2021
  24. practicalswift commented at 6:39 am on April 14, 2021: contributor

    cr ACK 5bc16d28e73a068382e66fce9ffcb604a3db34dc: patch looks correct

    Thanks a lot for doing this @hebasto: the introduction of -Werror=implicit-fallthrough in our project combined with proper [[fallthrough]] annotations will most likely kill the “unintended fallthroughs” bug class for good.

    Killing bug instances is cool, but killing entire bug classes is even cooler! :)

  25. DrahtBot added the label Needs rebase on Jun 17, 2021
  26. hebasto force-pushed on Jun 17, 2021
  27. hebasto commented at 4:01 pm on June 17, 2021: member
    Rebased 5bc16d28e73a068382e66fce9ffcb604a3db34dc -> 45dc9f383cf46a556fefa609dc88135cfe55b5bc (pr21430.03 -> pr21430.04) due to the conflict with #22258.
  28. DrahtBot removed the label Needs rebase on Jun 17, 2021
  29. practicalswift commented at 8:00 pm on June 17, 2021: contributor
    cr re-ACK 45dc9f383cf46a556fefa609dc88135cfe55b5bc
  30. in configure.ac:1249 in 45dc9f383c outdated 
    1245@@ -1245,7 +1246,7 @@ AC_LINK_IFELSE(
1246 
1247 AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM], [std::system or ::wsystem])
1248 
1249-LEVELDB_CPPFLAGS=
1250+LEVELDB_CPPFLAGS="-DFALLTHROUGH_INTENDED=@<:@@<:@fallthrough@:>@@:>@"
    fanquake commented at 8:51 am on June 30, 2021:
    Shouldn’t this be done in Makefile.leveldb.include?
    hebasto commented at 4:42 pm on July 3, 2021:
    Thanks! Updated.
  31. fanquake commented at 8:52 am on June 30, 2021: member
    Concept ACK.
  32. DrahtBot added the label Needs rebase on Jul 1, 2021
  33. hebasto force-pushed on Jul 3, 2021
  34. hebasto commented at 4:41 pm on July 3, 2021: member

    Updated 45dc9f383cf46a556fefa609dc88135cfe55b5bc -> ebc36d94cbc927a5c68873b92442937c420f317f (pr21430.04 -> pr21430.05):

  35. DrahtBot removed the label Needs rebase on Jul 3, 2021
  36. Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough 014110c47d
  37. build: Add -Werror=implicit-fallthrough compile flag 3c4c8e79ba
  38. in src/Makefile.leveldb.include:14 in ebc36d94cb outdated 
    10@@ -11,6 +11,7 @@ EXTRA_LIBRARIES += $(LIBMEMENV_INT)
11 LIBLEVELDB += $(LIBLEVELDB_INT) $(LIBCRC32C)
12 LIBMEMENV += $(LIBMEMENV_INT)
13 
14+LEVELDB_CPPFLAGS += -DFALLTHROUGH_INTENDED=[[fallthrough]]
    fanquake commented at 1:00 am on July 5, 2021:
    This should be in LEVELDB_CPPFLAGS_INT and down with the other defines.
    hebasto commented at 6:03 am on July 5, 2021:
    Thanks! Updated.
  39. hebasto force-pushed on Jul 5, 2021
  40. hebasto commented at 6:02 am on July 5, 2021: member

    Updated ebc36d94cbc927a5c68873b92442937c420f317f -> 3c4c8e79baf02af97ba1502189f649b04ef2198d (pr21430.05 -> pr21430.06, diff):

  41. fanquake approved
  42. fanquake commented at 4:33 am on July 9, 2021: member
    ACK 3c4c8e79baf02af97ba1502189f649b04ef2198d - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a [[fallthrough]] or FALLTHROUGH_INTENDED.
  43. theStack commented at 7:46 pm on July 12, 2021: member
    Concept ACK
  44. jarolrod commented at 3:46 pm on July 17, 2021: member

    ACK 3c4c8e79baf02af97ba1502189f649b04ef2198d

    This is nice to be able to detect fallthroughs going forward. Appropriate since GCC 7 is the current minimum supported GCC.

    Tested that the codebase itself will throw the warning by following the same step of removing one of the added [[fallthrough]] statements as outlined here: #21430#pullrequestreview-702673309

  45. hebasto commented at 4:30 pm on July 17, 2021: member

    @practicalswift @MarcoFalke

    Do you mind making another (final?) review?

  46. theStack approved
  47. theStack commented at 1:42 am on July 18, 2021: member

    ACK 3c4c8e79baf02af97ba1502189f649b04ef2198d

    Tested that removing an arbitrary [[fallthrough]] statment in the codebase leads to either a warning (default configuration) or an error (if configured with --enable-werror) with clang 11.0.1-2 (Debian bullseye/sid), e.g.:

     0hash.cpp:52:9: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
 1        case 2:
 2        ^
 3hash.cpp:52:9: note: insert '[[fallthrough]];' to silence this warning
 4        case 2:
 5        ^
 6        [[fallthrough]];
 7hash.cpp:52:9: note: insert 'break;' to avoid fall-through
 8        case 2:
 9        ^
10        break;
111 error generated.

    By the way, any reason we don’t --enable-werror by default? Being on the cautious side would seem to be a more sane default to me.

  48. fanquake merged this on Jul 18, 2021
  49. fanquake closed this on Jul 18, 2021
  50. MarcoFalke commented at 7:15 am on July 18, 2021: member

    By the way, any reason we don’t –enable-werror by default? Being on the cautious side would seem to be a more sane default to me.

    There might be unknown warnings in future releases of compilers that we are not aware of. Also, compilers might produce false warnings if they have bugs. So we only error on well tested and selected warnings.

  51. hebasto deleted the branch on Jul 18, 2021
  52. sidhujag referenced this in commit ea5c504276 on Jul 23, 2021
  53. gwillen referenced this in commit 2fa422afec on Jun 1, 2022
  54. DrahtBot locked this on Aug 18, 2022


hebasto DrahtBot practicalswift MarcoFalke fanquake theStack jarolrod

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me