Fix Windows build with --enable-werror #20586

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:201206-win changing 4 files +15 −7
  1. hebasto commented at 10:02 PM on December 6, 2020: member

    This PR makes possible to cross-compile Windows build with --enable-werror --enable-suppress-external-warnings. Some problems are fixed, others are silenced.

    Also --enable-werror is enabled for Cirrus CI Windows build (the last one on Cirrus CI without --enable-werror).

  2. hebasto commented at 10:03 PM on December 6, 2020: member

    cc @fanquake @MarcoFalke @vasild

  3. DrahtBot added the label Build system on Dec 6, 2020
  4. DrahtBot added the label GUI on Dec 6, 2020
  5. DrahtBot commented at 10:48 PM on December 6, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  6. DrahtBot cross-referenced this on Dec 6, 2020 from issue build: Do not repeat warning names in -Werror=... options by hebasto
  7. DrahtBot cross-referenced this on Dec 7, 2020 from issue Replace boost::filesystem with std::filesystem by kiminuo
  8. MarcoFalke removed the label GUI on Dec 7, 2020
  9. in src/fs.cpp:161 in 06ff47560e outdated 
     147 | @@ -148,7 +148,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
 148 |  #ifdef __GLIBCXX__
 149 |  
 150 |  // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
 151 | -
 152 | +#if defined(__GNUC__) && !defined(__clang__)
 153 | +#pragma GCC diagnostic push
 154 | +#pragma GCC diagnostic ignored "-Wswitch"
 155 | +#endif
 156 |  static std::string openmodeToStr(std::ios_base::openmode mode)
    vasild commented at 9:53 AM on December 7, 2020:

    This will disable -Wswitch for any GCC compiler (even latest on Linux). Shouldn't we restrict that somehow? Maybe check if WIN32 is defined or for a specific GCC version?

    vasild commented at 12:57 PM on December 7, 2020:

    Oh, it is already inside #ifdef WIN32, sorry for the noise.

    laanwj commented at 6:18 AM on August 27, 2021:

    I was about to ask this, but yes, it is already a (temporary, I guess) WIN32-specific workaround.

  10. in configure.ac:438 in 06ff47560e outdated 
     396 | +
 397 | +  dnl -Wreturn-type is broken in GCC for MinGW-w64.
 398 | +  dnl https://sourceforge.net/p/mingw-w64/bugs/306/
 399 | +  AX_CHECK_COMPILE_FLAG([-Werror=return-type], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=return-type"], [], [$CXXFLAG_WERROR],
 400 | +                        [AC_LANG_SOURCE([[#include <cassert>
 401 | +                                          int f(){ assert(false); }]])])
    vasild commented at 9:55 AM on December 7, 2020:

    This is good and will work fine.

    I am just curious, does that mingw bug 306 manifest if assert(false) is replaced with abort()?

    hebasto commented at 12:25 PM on December 7, 2020:

    Did not dive in details, but it seems to be dealing with an internal attribute of noreturn.

  11. laanwj commented at 12:52 PM on December 7, 2020: member

    I'm a bit divided on whether building on every platform/compiler combination 'with -Werror' is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.

    In general Werror is a curse.

  12. MarcoFalke commented at 1:06 PM on December 7, 2020: member

    Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

  13. vasild approved
  14. vasild commented at 1:10 PM on December 7, 2020: contributor

    ACK 06ff4756

    Sometimes important warnings are emitted only on some platforms, so I think it is good to use -Werror on as most platforms as possible.

    If keeping Windows builds warning-free turns out to be too big burden then the NO_WERROR=1 can be added back.

  15. hebasto commented at 1:11 PM on December 7, 2020: member

    I'm a bit divided on whether building on every platform/compiler combination 'with -Werror' is actually a goal of the project. I mean, I think this is impossible, and will result in a lot of warning fixing for the sake of warning fixing.

    In general Werror is a curse.

    Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

    Fair enough.

    The goal of this PR (combined with #20544) is to prevent changes that introduce new warnings.

    Anyway, feel free to close this.

  16. DrahtBot commented at 4:09 PM on December 9, 2020: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

    🕵️ @practicalswift has been requested to review this pull request as specified in the REVIEWERS file.

  17. practicalswift commented at 4:22 PM on December 9, 2020: contributor

    Concept ACK

  18. hebasto force-pushed on Dec 10, 2020
  19. hebasto commented at 12:06 PM on December 10, 2020: member

    Updated 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 -> 926f6d82a31b2d2c82b830b2b672fa38890b8590 (pr20586.01 -> pr20586.02, diff). @MarcoFalke

    Tend to agree with @laanwj, especially if the workarounds will make already hard to understand files like fs.cpp even more verbose.

    Only one-line change in fs.cpp now.

  20. in src/fs.cpp:154 in 926f6d82a3 outdated 
     151 | +// reference: https://github.com/gcc-mirror/gcc/blob/releases%2Fgcc-7.3.0/libstdc%2B%2B-v3/include/std/fstream#L270
 152 |  
 153 |  static std::string openmodeToStr(std::ios_base::openmode mode)
 154 |  {
 155 | -    switch (mode & ~std::ios_base::ate) {
 156 | +    switch (static_cast<int>(mode & ~std::ios_base::ate)) { // casting is required to suppress -Wswitch warnings
    vasild commented at 1:08 PM on December 10, 2020:

    std::ios_base::ate and friends are defined as unsigned int on my platform. What is the warning that this tries to fix?

    hebasto commented at 1:47 PM on December 10, 2020:

    std::ios_base::ate and friends are defined as unsigned int on my platform.

    https://github.com/gcc-mirror/gcc/blob/releases/gcc-7.3.0/libstdc%2B%2B-v3/include/bits/ios_base.h#L111-L122

    What is the warning that this tries to fix?

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

    -Wswitch ... case labels outside the enumeration range also provoke warnings when this option is used (even if there is a default label).

    vasild commented at 2:21 PM on December 10, 2020:

    Ok, we are fine because if some of the case values is out of the range of int, then we would get a warning:

    void f(unsigned int x)
{
    const unsigned int y = 3000000000U;

    switch (static_cast<int>(x)) { 
    case y:
        std::cout << "foo\n";
        break;
    } 
}

    error: case value evaluates to 3000000000, which cannot be narrowed to type 'int' [-Wc++11-narrowing]

    from clang 12 with -Wall

    luke-jr commented at 1:03 AM on December 14, 2020:

    NACK, the standard doesn't guarantee this fits in an int...

    Can you use a pragma to disable the warning here?

    hebasto commented at 5:11 PM on December 19, 2020:

    Done.

  21. vasild approved
  22. vasild commented at 2:22 PM on December 10, 2020: contributor

    ACK 926f6d82a31b2d2c82b830b2b672fa38890b8590

  23. luke-jr changes_requested
  24. hebasto force-pushed on Dec 19, 2020
  25. hebasto commented at 5:10 PM on December 19, 2020: member

    Reverted back 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 <- 926f6d82a31b2d2c82b830b2b672fa38890b8590 (pr20586.01 <- pr20586.02) as was requested by @luke-jr.

    This variant has been already ACKed by @vasild.

  26. DrahtBot cross-referenced this on Dec 22, 2020 from issue Use std::filesystem. Remove Boost Filesystem & System by fanquake
  27. DrahtBot cross-referenced this on Feb 2, 2021 from issue build: build fuzz tests by default by danben
  28. DrahtBot added the label Needs rebase on Feb 8, 2021
  29. DrahtBot removed the label Needs rebase on Feb 19, 2021
  30. hebasto cross-referenced this on Mar 4, 2021 from issue windows: new -Wreturn-type warnings after #19203 by fanquake
  31. practicalswift commented at 7:37 AM on March 6, 2021: contributor

    cr ACK 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464: patch looks correct

    FWIW, having this in master would have prevented the issue #21355 pre-merge :)

  32. in src/fs.cpp:159 in 83b4c4f96b outdated 
     147 | @@ -148,7 +148,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
 148 |  #ifdef __GLIBCXX__
 149 |  
 150 |  // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
 151 | -
 152 | +#if defined(__GNUC__) && !defined(__clang__)
 153 | +#pragma GCC diagnostic push
 154 | +#pragma GCC diagnostic ignored "-Wswitch"
    MarcoFalke commented at 7:40 AM on March 6, 2021:

    Is this commit " Fix Windows build with --enable-werror on Ubuntu Bionic (gcc 7.5) " still needed?

    We no longer use mingw on bionic (only focal)

    hebasto commented at 7:53 AM on March 6, 2021:

    ~Right. #21036 made it possible.~

    ~Going to rebase the pr and remove that commit.~

    hebasto commented at 8:20 AM on March 6, 2021:

    @MarcoFalke It is still required to prevent -Werror=switch warnings on Focal.

    Therefore, technically, no changes are required in the patch.

  33. DrahtBot cross-referenced this on Mar 17, 2021 from issue build: Add convenient BITCOIN_TRY_ADD_COMPILE_FLAG macro by hebasto
  34. DrahtBot cross-referenced this on Mar 17, 2021 from issue build: Add -Werror=unused... compile flags by hebasto
  35. DrahtBot cross-referenced this on Mar 18, 2021 from issue Move external signer out of wallet module by Sjors
  36. DrahtBot cross-referenced this on Apr 3, 2021 from issue rpc, gui: bumpfee signer support by Sjors
  37. Fix Windows build with --enable-werror on Ubuntu Focal c713bb2b24
  38. ci: Make Cirrus CI Windows build with --enable-werror b367745cfe
  39. hebasto force-pushed on Apr 13, 2021
  40. hebasto commented at 7:25 AM on April 13, 2021: member

    Updated 06ff47560ecb1d6a80e9fbd2f169f6ed8f31c464 -> b367745cfe19f6de3f44b3adc90fa08e36e44bb6 (pr20586.01 -> pr20586.03):

  41. vasild approved
  42. vasild commented at 8:45 AM on April 13, 2021: contributor

    ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6

  43. DrahtBot cross-referenced this on Apr 13, 2021 from issue build: Add -Werror=missing-noreturn by hebasto
  44. DrahtBot cross-referenced this on May 8, 2021 from issue Add minisketch subtree and integrate in build/test by sipa
  45. jarolrod commented at 5:40 AM on June 6, 2021: member

    ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6

    Tested on Linux cross-compiling to Windows passing --enable-werror to configure

    Master:

    script/standard.cpp:215:26: error: control reaches end of non-void function [-Werror=return-type]
  215 |     std::vector<valtype> vSolutions;

    PR: Builds fine and built binaries work fine on Windows 10

  46. practicalswift commented at 11:09 AM on June 6, 2021: contributor

    cr ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6: patch looks correct

  47. hebasto commented at 4:02 PM on June 17, 2021: member

    @laanwj @MarcoFalke @luke-jr

    Do you mind having another look at this PR?

  48. in src/fs.cpp:159 in b367745cfe 
     153 | @@ -154,7 +154,10 @@ std::string get_filesystem_error_message(const fs::filesystem_error& e)
 154 |  #ifdef __GLIBCXX__
 155 |  
 156 |  // reference: https://github.com/gcc-mirror/gcc/blob/gcc-7_3_0-release/libstdc%2B%2B-v3/include/std/fstream#L270
 157 | -
 158 | +#if defined(__GNUC__) && !defined(__clang__)
 159 | +#pragma GCC diagnostic push
 160 | +#pragma GCC diagnostic ignored "-Wswitch"
    MarcoFalke commented at 8:41 AM on June 23, 2021:

    Will this be needed after the boost::fs removal?

    hebasto commented at 9:19 AM on June 23, 2021:

    I think no, as this bunch of code should go: https://github.com/bitcoin/bitcoin/blob/d6e0d78c31557660274ef53cac912c468eecbe2d/src/fs.h#L66-L69

  49. DrahtBot cross-referenced this on Jun 24, 2021 from issue [TESTBED][NO-MERGE][POC] Use std::filesystem. Remove Boost Filesystem & System by kiminuo
  50. fanquake cross-referenced this on Jul 18, 2021 from issue build: fix -Wreturn-type compiler warnings (Win64) by jonatack
  51. laanwj commented at 6:20 AM on August 27, 2021: member

    I'm not a big fan in general of pragmas or fixing warnings for the sake of fixing warnings but this seems minimal enough. Code review ACK b367745cfe19f6de3f44b3adc90fa08e36e44bb6

  52. laanwj merged this on Aug 27, 2021
  53. laanwj closed this on Aug 27, 2021
  54. hebasto deleted the branch on Aug 27, 2021
  55. sidhujag referenced this in commit 4fe76c802b on Aug 28, 2021
  56. str4d cross-referenced this on Dec 6, 2021 from issue Backport changes to `src/fs.{cpp,h}` by str4d
  57. bitcoin locked this on Aug 27, 2022
Contributors
hebastoDrahtBotvasildlaanwjMarcoFalkepracticalswiftluke-jrjarolrod
Labels
Build system
Linked (view graph)
#21036 gitian: Bump descriptors to Focal for 22.0#21355 windows: new -Wreturn-type warnings after #19203#22484 build: fix -Wreturn-type compiler warnings (Win64)
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: 2026-05-19 06:53 UTC