mock_process/* output
#34791
This is a follow-up to #33929.
The mock_process/* test cases, which serve as helpers for system_tests rather than actual tests, are invoked in a way that suppresses unsolicited output to stdout or stderr to keep the test results reproducible.
However, in debug builds, the Windows CRT still prints false-positive memory leak dumps to stderr.
This PR handles this specific case and documents the other suppressions.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Not sure about disabling this globally, if it only causes issues for an experimental platform, in a nightly build CI?
There’s also no inline docs to explain why this is disabled.
mock_process anyway. The purpose of mock_process is to only produce exit code and std/err output.
Not sure about disabling this globally, if it only causes issues for an experimental platform, in a nightly build CI?
Alternatively, the BOOST_TEST_DETECT_MEMORY_LEAK environment variable can be set in the nightly CI.
There’s also no inline docs to explain why this is disabled.
Added.
Also, can you add a link to the upstream bug report?
It doesn’t seem like a bug to me. Some relevant discussion is here: https://github.com/boostorg/test/issues/258.
Alternatively, the BOOST_TEST_DETECT_MEMORY_LEAK environment variable can be set in the nightly CI.
That seems like the proper solution here, with it’s usage documentation explaining that memory leaks are expected on Windows, if this isn’t considered a bug or issue.
(Copying my comment from the nightly repo)
This still feels like it’s being fixed at the wrong place to me. If a single .cpp needs a define under certain build conditions, can we not just detect them there and set the define there as necessary? Rather than a global env var in a far-away c-i descriptor.
Something like (at the top of test/system_tests.cpp):
0diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
1index bd57661b051..bb5bc40cba5 100644
2--- a/src/test/system_tests.cpp
3+++ b/src/test/system_tests.cpp
4@@ -5,6 +5,11 @@
5
6 #include <bitcoin-build-config.h> // IWYU pragma: keep
7
8+#if defined(_MSC_VER) && defined(_DEBUG)
9+// Some explanation for why this is necessary
10+#define BOOST_TEST_DETECT_MEMORY_LEAK 0
11+#endif
12+
13 #include <common/run_command.h>
14 #include <test/util/common.h>
15 #include <test/util/setup_common.h>
Or am I misunderstanding the problem?
Something like (at the top of
test/system_tests.cpp):…
The Boost docs describe only a command line option and an environment variable.
UPDATE 1: However, something similar can be achieved using Windows CRT-specific macros.
UPDATE 2. And it should be done in test/mock_process.cpp, not in test/system_tests.cpp.
It doesn’t seem like a bug to me. Some relevant discussion is here: boostorg/test#258.
I think that introduced a real leak:
0BOOST_AUTO_TEST_CASE( do_leak_test ) { int *i = new int; }
Whereas here, there is no leak in our code, but in boost or in the MSVC debug stdlib?
I was assuming that usage of BOOST_FAIL caused an unclean abort without cleanups. If that’s not the case, then yeah, we should figure out what’s actually leaking.
Edit: presumably that would show in sanitizers/valgrind too, though.
I was assuming that usage of
BOOST_FAILcaused an unclean abort without cleanups. If that’s not the case, then yeah, we should figure out what’s actually leaking.Edit: presumably that would show in sanitizers/valgrind too, though.
I’ve minimized the code to this example:
0#include <clientversion.h>
1
2#include <boost/test/unit_test.hpp>
3
4#include <stdexcept>
5
6static int f(int v)
7{
8 if (v < 0) {
9 throw std::runtime_error{
10 // this causes memory leaks
11 FormatFullVersion()
12 };
13 }
14 return 0;
15}
16
17BOOST_AUTO_TEST_SUITE(memory_leaks_tests)
18BOOST_AUTO_TEST_CASE(memory_leaks)
19{
20 BOOST_CHECK_EQUAL(f(0), 0);
21}
22BOOST_AUTO_TEST_SUITE_END()
_CrtDumpMemoryLeaks is called before all static objects are destructed?
/fsanitize=address. Running build\bin\Debug\test_bitcoin.exe --run_test=mock_process didn’t reveal any issues.
I’ve minimized the code to this example:
Is this the same bug? The pull request description and patch seem to show that the bug happens inside the mock_process test, however that test has no throw? In system_tests, RunCommandParseJSON throws, so it seems like the bug is in system_tests?
In any case your reproducer looks like a stdlib bug. Does it happen with clang-cl? If not, we may consider going with https://github.com/bitcoin/bitcoin/pull/31507
I’ve minimized the code to this example:
Is this the same bug? The pull request description and patch seem to show that the bug happens inside the mock_process test, however that test has no throw? In system_tests,
RunCommandParseJSONthrows, so it seems like the bug is in system_tests?
I’ve also made two additional observations:
The memory leaks dump is printed for any other test.
If the test_bitcoin target sources are limited strictly to main.cpp and mock_process.cpp, the memory leak dump no longer appears.
Does it happen with clang-cl?
It does.
30+ // Runtime options to suppress all additional Boost.Test output:
31+ // --detect_memory_leaks=0 - disables memory leak detection, preventing Windows
32+ // CRT memory leak dumps for BOOST_FAIL in "Debug" builds.
33+ // --log_level=nothing - disables logging.
34+ // --report_level=no - disables test report.
35+ return {boost::unit_test::framework::master_test_suite().argv[0], "--detect_memory_leaks=0", "--log_level=nothing", "--report_level=no", "--run_test=mock_process/" + name};
nit: You can make this multi-line (with a trailing comma) and add the comments in-line?
0 return {boost::unit_test::framework::master_test_suite().argv[0], "--detect_memory_leaks=0", "--log_level=nothing", "--report_level=no", "--run_test=mock_process/" + name,};
25@@ -26,7 +26,17 @@ BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
26
27 static std::vector<std::string> mock_executable(std::string name)
28 {
29- return {boost::unit_test::framework::master_test_suite().argv[0], "--log_level=nothing", "--report_level=no", "--run_test=mock_process/" + name};
30+ // Invoke the mock_process/* test case with all unsolicited output suppressed.
31+ return {
32+ boost::unit_test::framework::master_test_suite().argv[0],
33+ // Disable false-positive memory leak dumps to stderr in debug builds.
The `mock_process/*` test cases, which serve as helpers for
`system_tests` rather than actual tests, are invoked in a way that
suppresses unsolicited output to stdout or stderr to keep the test
results reproducible.
However, in debug builds, the Windows CRT still prints false-positive
memory leak dumps to stderr.
This change handles this specific case and documents the other
suppressions.
… it can be suppressed there using the already available mechanisms…
The drawback of this approach was already highlighted in https://github.com/hebasto/bitcoin-core-nightly/pull/207#issuecomment-4033428392.
The drawback of this approach was already highlighted in
Not sure I agree, a drawback here seems ok, given it’s confined to a single job, in a nightly repo, and only exists for the duration of time during which the underlying issue is being properly debugged/identified/fixed.
I would also say, given the intent to try and swap our release binaries to use UCRT (#33539), it seems like prior to swapping is the right time to investigate, and ideally report/fix (upstream, or here), issues we already know about.
I’m not sure that UCRT-based release binaries are affected.
25@@ -26,7 +26,18 @@ BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
26
27 static std::vector<std::string> mock_executable(std::string name)
28 {
29- return {boost::unit_test::framework::master_test_suite().argv[0], "--log_level=nothing", "--report_level=no", "--run_test=mock_process/" + name};
30+ // Invoke the mock_process/* test case with all unsolicited output suppressed.
31+ return {
32+ boost::unit_test::framework::master_test_suite().argv[0],
33+ // Disable false-positive memory leak dumps to stderr
34+ // in debug builds when using the Windows UCRT.
Windows, GCC, MSVCRT and Windows, GCC, UCRT?)
FWIW, I’ve applied the following patch:
0@@ -20,7 +20,7 @@ mingw32_release_CXXFLAGS=$(mingw32_release_CFLAGS)
1 mingw32_debug_CFLAGS=-O1 -g
2 mingw32_debug_CXXFLAGS=$(mingw32_debug_CFLAGS)
3
4-mingw32_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC
5+mingw32_debug_CPPFLAGS=-D_DEBUG -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC
6
7 mingw32_cmake_system_name=Windows
8 # Windows 10
Then I cross-compiled test_bitcoin.exe for both x86_64-w64-mingw32 and x86_64-w64-mingw32ucrt. Both binaries failed to run on Windows. I didn’t investigate further for now.
I’m not sure that UCRT-based release binaries are affected.
I’m not quite sure of your point. If we switch to UCRT release binaries, then the Debug binaries also need to work/be maintained?
--catch_system_errors=no and then use vanilla C++ to set the exit code via std::exit(code)?
I’m not sure that UCRT-based release binaries are affected.
I’m not quite sure of your point. If we switch to UCRT release binaries, then the Debug binaries also need to work/be maintained?
I agree. However, the _DEBUG macro, which enables the debug version of the C run-time library, has never been used for cross-compiling. Perhaps this was an oversight.
UPDATE. See #34791 (review).
If this is too controversial, an alternative could be to just set
--catch_system_errors=noand then use vanilla C++ to set the exit code viastd::exit(code)?
I’ve just tested this approach. Unfortunately, it doesn’t work.
I’ve just tested this approach. Unfortunately, it doesn’t work.
What was the error message, for reference? Just wondering, the patch here seems fine to merge as-is to work around an issue that probably no one real person is running into.
I’ve just tested this approach. Unfortunately, it doesn’t work.
What was the error message, for reference? Just wondering…
The suggested patch doesn’t change behavior. When a test is executed, the memory leak dump is printed.
Concept NACK any change that touches platforms that don’t need this change…
From the Boost.Test docs:
The only platform which supports memory leak detection is Microsoft Visual Studio family of compilers in debug builds.
… especially given the underlying issue doesn’t seem to be properly identified?
There are objects in our code base that are initialized before main() and destructed after the program execution end. For example, this global instance: https://github.com/bitcoin/bitcoin/blob/390e7d61bd531505bb3d13f38316c282b85ed1dd/src/common/args.cpp#L40
This is not a problem when running normal program, such as bitcoind.exe, because the debug C runtime automatically calls _CrtDumpMemoryLeaks after all planned destructions have happened.
However, the Boost.Test framework has its own detect_memory_leaks feature managed by its internal execution monitor. It calls _CrtDumpMemoryLeaks before the program exits, which inevitably flags the memory allocated for these global instances as leaks.