GetTotalRAM() on other platforms where it was tested to work.GetTotalRAM() unit test on unsupported platforms.Prior discussion: #33333 (review)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33435.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
112@@ -113,10 +113,10 @@ int GetNumCores()
113
114 std::optional<size_t> GetTotalRAM()
115 {
116- auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
117+ [[maybe_unused]] auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
118 #ifdef WIN32
119 if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
120-#elif defined(__linux__) || defined(__APPLE__)
121+#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__)
0#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__) || defined(__illumos__)
Here are the relevant docs:
Based on https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/common/system.cpp#L79 I also suggest something similar:
0#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__)
I have also tested locally on the following system:
0$ uname -snrv
1SunOS openindiana 5.11 illumos-ee653ea2dd
If ... || defined(__illumos__) is not appended, the following test fails:
0$ ./build/bin/test_bitcoin -t system_tests/total_ram
1Running 1 test case...
2./test/system_tests.cpp(23): error: in "system_tests/total_ram": check GetTotalRAM() >= 1000_MiB has failed [std::nullopt < 1048576000]
3
4*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
Perfect, that’s why I have added the test so that we can detect these.
Does it display the correct value if we print GetTotalRAM()?
0diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
1--- a/src/test/system_tests.cpp (revision 50ba74ae2cb97aa9052f4e20385dff3afdb6d7cd)
2+++ b/src/test/system_tests.cpp (revision c968ae0b75785f6cc70d80f649a45b827a80b6b0)
3@@ -21,6 +21,7 @@
4 BOOST_AUTO_TEST_CASE(total_ram)
5 {
6 BOOST_CHECK_GE(GetTotalRAM(), 1000_MiB);
7+ std::cout << "GetTotalRAM() = " << GetTotalRAM() << std::endl;
8
9 if constexpr (SIZE_MAX == UINT64_MAX) {
10 // Upper bound check only on 64-bit: 32-bit systems can reasonably have max memory,
FreeBSD and OpenBSD seem to be detected correctly based on https://github.com/l0rinc/bitcoin/actions/runs/17870295239/job/50822293463?pr=39 (still waiting for NetBSD)
Does it display the correct value if we print
GetTotalRAM()?
Yes, it does.
NetBSD also finished successfully: https://github.com/l0rinc/bitcoin/actions/runs/17870748757/job/50823702246?pr=39
Thanks @hebasto for finding it, thanks @vasild for the quick PR.
I’m working on trying to reproduce this on the mentioned platforms, but there’s a reason I didn’t add them originally since I couldn’t test them via godbolt (https://xania.org/202506/how-compiler-explorer-works indicates they don’t have native BSD or s390x servers). @vasild if you edit again, can you please add @hebasto as a coauthor for the change?
Concept ACK for both changes in the PR (the maybe unused + adding BSD), I’m trying to find a way to test it myself before fully ACKing.
I was able to reproduce the problem with FreeBSD by adding a dummy CI job:
error: unused variable ‘clamp’
After applying the PR the test passes successfully:
test/system_tests.cpp(23): info: check GetTotalRAM() >= 1000_MiB has passed GetTotalRAM() = 4258750464 test/system_tests.cpp(29): info: check GetTotalRAM() < 10'000'000_MiB has passed
Which coincides with the set memory: 4G.
Based on the BSD definitions in the same file I left a suggestion but I’m also fine with merging as-is.
ACK 2b6497b03585922519a16f782be8f6c83e55ab29
#ifdefs as in the GetTotalRAM() implementation. Otherwise, this test will fail on any unmentioned system.
Otherwise, this test will fail on any unmentioned system
That’s exactly why it was added so that we can detect and cover those. Is it too strict?
Otherwise, this test will fail on any unmentioned system
That’s exactly why it was added so that we can detect and cover those. Is it too strict?
We usually avoid breaking people’s tests just to collect coverage data :)
If we do want to make sure unsupported systems pass gracefully, can we at least add a warning test log for those cases?
I’d say that the total_ram test should be:
ctest can clearly distinguish and report the skipped test in the statistics.
f011c2c (#39) on top of the PR?
Please add @hebasto as a coauthor.
2b6497b035...8fcf71ca00: pick suggestions
cherry-pick l0rinc/bitcoin@f011c2c
done with some minor changes, see git range-diff f011c2cd~..f011c2cd 8fcf71ca00~..8fcf71ca00
ACK 8fcf71ca005449e639f189dec7ec0163a07e6d37
Reproduced the unused clamp on FreeBSD and that the fix returns the correct memory on FreeBSD and NetBSD and OpenBSD as well (__illumos__ was tested by @hebasto).
I have also tested that on systems that don’t support the memory query the test is gracefully skipped and reported as skipped.
Thanks for the quick fix.
122+ defined(__FreeBSD__) || \
123+ defined(__NetBSD__) || \
124+ defined(__OpenBSD__) || \
125+ defined(__illumos__) || \
126+ defined(__linux__)
127 if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
By the way:
0warning: implicit conversion changes signedness: 'long' to 'unsigned long long' [-Wsign-conversion]
1... return clamp(1ULL * p * s);
2... ~ ^
changing it to
0clamp(static_cast<uint64_t>(p) * static_cast<uint64_t>(s))
silences that. -Wsign-conversion also enabled by -Wconversion is not used by Bitcoin Core, so we don’t see it when compiling. In this case the code is ok because values are explicitly checked that they are greater than 0 before doing the implicit conversion. Maybe do nothing.
0@@ -0,0 +1,33 @@
1+// Copyright (c) 2025-present The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+//
0// file COPYING or https://opensource.org/license/mit/.
121+#elif defined(__APPLE__) || \
122+ defined(__FreeBSD__) || \
123+ defined(__NetBSD__) || \
124+ defined(__OpenBSD__) || \
125+ defined(__illumos__) || \
126+ defined(__linux__)
4ebf456dc589a993faa73b8b377c0d1919fbd577
Could you please update the commit message so it reflects the changes?
This patch achieves two things:
1. Fix unused variable warning (https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362493046)
2. Enable GetTotalRAM() on other platforms where it was tested to work.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
when `GetTotalRAM` returns an `std::nullopt` now we're getting:
```
The following tests did not run:
106 - system_ram_tests (Skipped)
```8fcf71ca00...56791b5829: take suggestions
