fanquake
commented at 2:22 pm on September 1, 2022:
member
Now that CXXFLAGS are back in user control, I don’t think there’s a
reason to no-longer use our warning flags when CXXFLAGS has been
overriden (this includes, by default, when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant -Wno- options in CXXFLAGS.
theuni
commented at 7:03 pm on September 13, 2022:
member
As @fanquake mentioned, some of these are only warnings in older compilers. I’m going through them to double-check that they are indeed non-issues, as opposed to missed warnings in newer compilers.
(I’m ignoring the sdt.h warnings for now and focusing on what’s in our code. I will look at that header next.)
script/descriptor.cpp:1555:21: error: loop variable ‘keyspan’ of type ‘const Span’ creates a copy from type ‘const Span’ [-Werror,-Wrange-loop-analysis]
for (const auto keyspan : match->second) {
tl;dr: copies are allowed for POD types less than 64bytes.
We can confirm this by adding a dummy char foo[65] to Span, which causes newer clang to warn as expected.
That seems reasonable enough, but I think we may as well just fix it up to take a const ref to remove any ambiguity.
for this one:
script/descriptor.cpp: In function ‘std::unique_ptr<{anonymous}::DescriptorImpl> {anonymous}::InferScript(const CScript&, {anonymous}::ParseScriptContext, const SigningProvider&)’:
script/descriptor.cpp:1262:17: error: ‘’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
return {};
Though I’m having trouble recreating a minimal test-case that fails with gcc9 and succeeds with gcc10.
Changing these from return {} to return std::nullopt avoids the problem and makes the code more clear too, so again, I suggest simply appeasing the dumb compiler because there’s not really a reason not to.
For this one:
fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’:
fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers]
129 | _OVERLAPPED overlapped = {0};
This one looks legitimate. Suggest fixing by initializing to {}.
For this one:
test/fuzz/float.cpp: In function ‘void float_fuzz_target(FuzzBufferType)’:
test/fuzz/float.cpp:50:40: error: ‘tmp’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
uint64_t encoded = EncodeDouble(d);
(This applies to x86_64 as well)
Again, gcc10 is smart enough to figure this out, but gcc9 complains. Initializing to zero is harmless, so again I suggest just initializing to 0 to quiet the old compilers.
theuni
commented at 8:38 pm on September 13, 2022:
member
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I’m not at all confident in that change yet.
theuni
commented at 8:38 pm on September 13, 2022:
member
Concept ACK, btw :)
fanquake force-pushed
on Sep 14, 2022
fanquake
commented at 11:00 am on September 14, 2022:
member
This one looks legitimate. Suggest fixing by initializing to {}.
I had opened #26006, to see if anyone would jump in with the change, but at this point will just PR your commit separate to this.
For the remaining of the changes, I’ve rebased on top of your branch I think the changes are fairly uncontreversial (aside from sdt.h), and are better than adding -Wno-x for older compilers / CI jobs, which is better again than going the #pragma in our source code route. Although given they are changes to code, they can also be split out of here if wanted.
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I’m not at all confident in that change yet.
0xB10C
commented at 12:36 pm on September 14, 2022:
contributor
I also pushed a patch for systemtap in depends that gets rid of those compiler warnings. From what I can tell, the variadic is completely unused there, but I’m not at all confident in that change yet.
I too think that we don’t use the variadic and it’s Ok to drop it in the patch:
We use, for example, the DTRACE_PROBE6(context, event, a, b, c, d, e, f) macro which expands to STAP_PROBE6(provider,probe,parm1,parm2,parm3,parm4,parm5,parm6) expanding too _SDT_PROBE(provider, name, 6, (arg1, arg2, arg3, arg4, arg5, arg6)) and again expanding too (_SDT_ASM_BODY(provider, name, _SDT_ASM_ARGS, (n)) which is defined as _SDT_ASM_BODY(provider, name, pack_args, args, ...) containing the variadic. We only fill in the provider, name, pack_args, args and don’t use the variadic.
The the tracepoint tests in this task succeed. Also, for reference, this also came up in #25528 (comment) and the variadic was only recently re-added in sytemtap@ecab2afe. If needed, we could also have our own sdt.h at some point with everything we don’t use thrown out.
fanquake
commented at 2:02 pm on September 14, 2022:
member
I think it’s a nice improvement. Will PR if there’s any interest.
theuni
commented at 8:15 pm on September 14, 2022:
member
I too think that we don’t use the variadic and it’s Ok to drop it in the patch:
We use, for example, the DTRACE_PROBE6(context, event, a, b, c, d, e, f) macro which expands to STAP_PROBE6(provider,probe,parm1,parm2,parm3,parm4,parm5,parm6) expanding too _SDT_PROBE(provider, name, 6, (arg1, arg2, arg3, arg4, arg5, arg6)) and again expanding too (_SDT_ASM_BODY(provider, name, _SDT_ASM_ARGS, (n)) which is defined as _SDT_ASM_BODY(provider, name, pack_args, args, ...) containing the variadic. We only fill in the provider, name, pack_args, args and don’t use the variadic.
The the tracepoint tests in this task succeed. Also, for reference, this also came up in #25528 (comment) and the variadic was only recently re-added in sytemtap@ecab2afe. If needed, we could also have our own sdt.h at some point with everything we don’t use thrown out.
Note that not only do we do not use the variadic, it’s not even hooked up to anything. _SDT_ASM_BODY doesn’t use __VA_ARGS__ at all, so any extra args passed in would just be discarded anyway. I’m assuming this is a placeholder for some later upstream feature (or maybe something someone forgot to remove), but for now it’s doing nothing but triggering a warning.
DrahtBot
commented at 11:26 pm on September 14, 2022:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
maflcko
commented at 6:14 am on September 15, 2022:
member
Here’s a slightly more invasive change that eliminates the possibility of undefined behavior
Strictly speaking, this will keep the UB. Recall that an enum class is allowed to hold any integral value as long as it fits in the underlying type (even if the value is not named in the enum). So you’d have to re-add the assert to avoid UB (an uninitialized value), in which case the warning would likely re-appear.
theuni
commented at 1:12 pm on September 15, 2022:
member
Here’s a slightly more invasive change that eliminates the possibility of undefined behavior
Strictly speaking, this will keep the UB. Recall that an enum class is allowed to hold any integral value as long as it fits in the underlying type (even if the value is not named in the enum). So you’d have to re-add the assert to avoid UB (an uninitialized value), in which case the warning would likely re-appear.
Heh, I suppose I invited the pedantry there.
Correct, it wouldn’t eliminate UB potential entirely here, but it would put it on par with every other switch/case over an fully-handled enum class without a default in our codebase. I don’t think you’re suggesting adding default: assert(0) for the others? :)
As I see it it’d be strictly an improvement (though admittedly not bulletproof) as the intent is better communicated to callers, and would cleanly eliminate this warning. Would you NACK it as a PR?
fanquake referenced this in commit
f332c4f64d
on Sep 15, 2022
maflcko
commented at 2:36 pm on September 15, 2022:
member
I don’t think you’re suggesting adding default: assert(0) for the others? :)
sidhujag referenced this in commit
1c746cca25
on Sep 15, 2022
fanquake referenced this in commit
44a29758a0
on Oct 4, 2022
fanquake force-pushed
on Oct 4, 2022
sidhujag referenced this in commit
73e27f2fe2
on Oct 4, 2022
mruddy
commented at 1:44 am on November 1, 2022:
contributor
Concept ACK on enabling the warning messages when CXXFLAGS is set.
These changes would benefit what I was working on in #26393.
I’d probably support defaulting --enable-suppress-external-warnings, but that can be done later/separate.
I rebased the current 6 commits onto the current master, 43e813cab266eef42e622519836f171f6a18d426, and did a build with depends using:
CONFIG_SITE="$PWD/depends/x86_64-pc-linux-gnu/share/config.site" ./configure --enable-debug CXXFLAGS="-O0"
and got a ton of boost warnings related to -Wsuggest-override.
I went back and did another build using:
CONFIG_SITE="$PWD/depends/x86_64-pc-linux-gnu/share/config.site" ./configure --enable-suppress-external-warnings --enable-debug CXXFLAGS="-O0"
and got a clean build.
fanquake referenced this in commit
e1fb7381be
on Dec 10, 2022
hebasto
commented at 3:38 pm on December 10, 2022:
member
assert(0) doesn’t prevent this code from being hit in -DNDEBUG builds, so I believe this warning is correct.
There is also Assert(0), which shouldn’t have this static analysis problem.
Edit: Ok, maybe it does. Though annotating assertion_fail with [[noreturn]] might fix that.
Assert(0) combined with [[noreturn]] assertion_fail does fix the warning.
maflcko
commented at 12:38 pm on December 12, 2022:
Not sure if we want to maim code to please broken and ancient compilers
hebasto
commented at 10:12 am on January 24, 2023:
This only change is better than disabling a useful warning, isn’t it?
maflcko
commented at 10:20 am on January 24, 2023:
No. C++ allows for uninitialized values, and allows for valgrind to find code logic errors based on this.
I fail to see how using a warning emitted by a known-broken compiler is an excuse to worsen the code.
Either the compiler shouldn’t be used or the warning be disabled on the compiler.
maflcko
commented at 12:01 pm on February 1, 2023:
Looks like this (and other changes) can be dropped after #27013
bitcoin deleted a comment
on Jan 23, 2023
DrahtBot added the label
Needs rebase
on Jan 24, 2023
fanquake force-pushed
on Jan 24, 2023
maflcko dismissed
maflcko
commented at 10:20 am on January 24, 2023:
member
idk. Probably NACK?
DrahtBot removed the label
Needs rebase
on Jan 24, 2023
fanquake
commented at 11:32 am on January 24, 2023:
member
idk. Probably NACK?
Are you NACKING the whole (concept of the) pull, or the single fuzz change? I agree some of these changes are not ideal, and I would rather avoid the use of old/broken compilers entirely. I’ll continue to split out changes where they make sense, and look at fixing issues in another way.
The main issue here is the use of old/broken compilers in the CI, in combination with -Werror, as we’re essentially converting incorrect output from old/broken compilers, into progress blocking issues, as the CI will be red.
maflcko
commented at 11:49 am on January 24, 2023:
member
In the OP you mention that it is possible to pass “the relevant -Wno- option in CXXFLAGS.” So I wonder why you opted to change the code (https://github.com/bitcoin/bitcoin/pull/25972#discussion_r1045780114) instead of using your suggestion to simply disable the broken warning?
The main issue here is the use of old/broken compilers in the CI
I presume the reason we are using those compilers is, that they are the versions used when cross-compiling to Windows (by the average user, or by our guix build). So unless the average user, or at least the guix build is using a different version, we are stuck with them.
fanquake
commented at 11:57 am on January 24, 2023:
member
In the OP you mention that it is possible to pass “the relevant -Wno- option
If you’re happy for us to maintain various -Wno options throughout the CI configs, to suppress warnings from the broken compilers, I’m happy to convert to that approach. I’d say, in general, if we can make a single line code change, vs adding further configuration complexitiy to our CI, that would seem preferred? In the op I was also generally referring to 3rd party code, as that is what is packaged in depends, and is usually, the primary cause of copious amounts of warnings.
I presume the reason we are using those compilers is,
I’m not arguing against testing the older compilers, I’m saying I don’t think we should be testing them with -Werror, when we already know they are broken, going to produce garbage output, and cause pointless build failures.
maflcko
commented at 12:05 pm on January 24, 2023:
member
a single line code change, vs adding further configuration complexitiy to our CI, that would seem preferred?
I don’t think a single line code change is enough? The CI currently fails with:
0script/interpreter.cpp:1530:42: error: ‘ext_flag’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
11530|const uint8_t spend_type = (ext_flag <<1) + (have_annex ?1 : 0); // The low bit indicates whether an annex is present.
When the options are to refactor consensus code, potentially introducing a consensus bug, or disabling a harmless, but broken compiler warning, I think the answer should be clear? This is also the approach we take with sanitizer suppressions.
fanquake
commented at 12:07 pm on January 24, 2023:
member
I don’t think a single line code change is enough? The CI currently fails with:
A single line change was enough to fix all other warnings. The one you are referencing, clearly not, which is why the fix was split off into #26101 for further discussion. The other issue there is that we probably don’t want to turn off -Wmaybe-uninitialized warnings, for GCC 10, which is currently our release compiler.
maflcko
commented at 12:48 pm on January 24, 2023:
member
In any case, if this is currently not for merge (CI is red) and depending on another change, maybe close or mark as draft for now?
fanquake marked this as a draft
on Jan 24, 2023
fanquake
commented at 1:18 pm on January 24, 2023:
member
maybe close or mark as draft for now?
Drafted. This is based on #26945 and may use #26101. May also change the approach to suppressions instead.
fanquake referenced this in commit
eee2c28985
on Jan 26, 2023
DrahtBot added the label
Needs rebase
on Jan 26, 2023
fanquake force-pushed
on Jan 26, 2023
fanquake
commented at 1:58 pm on January 26, 2023:
member
Rebased for #26945 and rebased on top of #26101 for now/CI. However note that this is now failing with what is likely another false positive, in some recently introduced code:
0test/fuzz/partially_downloaded_block.cpp: In function ‘void partially_downloaded_block_fuzz_target(FuzzBufferType)’:
1test/fuzz/partially_downloaded_block.cpp:122:46: error: ‘iftmp.71’ may be used uninitialized in this function[-Werror=maybe-uninitialized] 2 pdb.m_check_block_mock = FuzzedCheckBlock( 3 ~~~~~~~~~~~~~~~~^
4 fail_check_block ?
5 ~~~~~~~~~~~~~~~~~~
6 std::optional<BlockValidationResult>{validation_result} :
7 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
8 std::nullopt);
9 ~~~~~~~~~~~~~
10cc1plus: all warnings being treated as errors
11make[2]: *** [Makefile:16735: test/fuzz/fuzz-partially_downloaded_block.o] Error 1
It’s clear that using -Werror with older/broken compilers is going to be impossible to maintain, and trying to suppress individual classes of warnings is a constantly moving target. Will PR some changes to improve this.
maflcko
commented at 2:39 pm on January 26, 2023:
member
Right, so the previous patch just happened to be a single-line fixup. Though, for newly written code you’ll harass devs with incorrect errors and force them to regress their code. So I’ll remain Approach NACK on the current state of this pull.
fanquake
commented at 2:54 pm on January 26, 2023:
member
you’ll harass devs with incorrect errors and force them to regress their code
No, the (global) use of -Werror with old, broken compilers, in the CI, would be harassing them. Which is what I alluded to fixing in my previous comment. Warnings in our code when built with depends (relases), have always existed, but have either been suppressed, or ignored entirely, by not actually turning on any warning flags. The point of this pull is to turn on the flags with depends (release) builds, so that you actually get (useful/meaningful) output. This is now possible given there are less warnings produced by the depends libs in general, and we have the infra in our build system to easily suppress warnings that are not coming from our own code.
Given the same source code, and two versions of the same compiler, I don’t see why we have decided that turning the CI red, when the older (known broken) compiler spits out a false-positive warning, is something we need to deal with. If devs/users want to compile their code with an older compiler, that’s fine, and we’ll continue to do our best to support that, but there is no-reason for that older compiler, producing an incorrect warning, to be some sort of blocker in our CI.
DrahtBot removed the label
Needs rebase
on Jan 26, 2023
sidhujag referenced this in commit
53564628e1
on Jan 26, 2023
fanquake force-pushed
on Feb 1, 2023
fanquake
commented at 3:28 pm on February 1, 2023:
member
Dropped some changes out of here, and rebased on top of #27013.
in
src/script/interpreter.cpp:1495
in
4242db9259outdated
1493 // An upgradable public key version (with a size not 32-byte) may
1494 // request a different key_version with a new sigversion.
1495 key_version = 0;
1496 break;
1497- default:
1498- assert(false);
hebasto
commented at 2:25 pm on September 11, 2023:
member
From what I can tell GCC for Windows is just broken, untill GCC 13 (i.e building locally with these changes).
On Ubuntu Mantic, the build produces the only warning:
0$ x86_64-w64-mingw32-gcc --version | head -11x86_64-w64-mingw32-gcc (GCC) 12-posix
2$ make -C depends HOST=x86_64-w64-mingw32
3$./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
4$ make >/dev/null
5qt/winshutdownmonitor.cpp: In static member function ‘static void WinShutdownMonitor::registerShutdownBlockReason(const QString&, HWND__*const&)’:
6qt/winshutdownmonitor.cpp:46:42: warning: cast between incompatible function types from ‘FARPROC’ {aka ‘long long int (*)()’} to ‘PSHUTDOWNBRCREATE’ {aka ‘int (*)(HWND__*, const wchar_t*)’} [-Wcast-function-type]
746| PSHUTDOWNBRCREATE shutdownBRCreate = (PSHUTDOWNBRCREATE)GetProcAddress(GetModuleHandleA("User32.dll"), "ShutdownBlockReasonCreate");
8|^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
maflcko
commented at 5:26 pm on September 20, 2023:
Not sure
Is this even still needed?
fanquake
commented at 2:10 pm on September 21, 2023:
The changes here will fail without it:
0torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
1torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized] 294 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
3 | ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4In file included from ./netaddress.h:18,
5 from ./torcontrol.h:11,
6 from torcontrol.cpp:6:
7./util/strencodings.h:184:7: note: ‘result’ was declared here
8184 | T result;
9 | ^~~~~~
10cc1plus: all warnings being treated as errors
11make[2]: *** [Makefile:11088: libbitcoin_node_a-torcontrol.o] Error 112make[2]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-w64-mingw32/src'13make[1]: *** [Makefile:20110: all-recursive] Error 114make[1]: Leaving directory '/ci_container_base/ci/scratch/build/bitcoin-x86_64-w64-mingw32/src'15make: *** [Makefile:814: all-recursive] Error 1
maflcko
commented at 2:15 pm on September 21, 2023:
Seems like your change may be fine here, but I think generally it will cause issues if this pull is going to be merged.
Imagine another place that needs error checking for exactly int. What are they supposed to do?
Seems better to include the workaround here, than to transform it into a blocker for another patch.
fanquake
commented at 9:32 am on September 26, 2023:
Switched to -Wno-error=maybe-uninitialized
hebasto referenced this in commit
2e1d4bf444
on Sep 22, 2023
Frank-GER referenced this in commit
5ca8948475
on Sep 25, 2023
fanquake force-pushed
on Sep 26, 2023
fanquake
commented at 9:33 am on September 26, 2023:
member
maflcko
commented at 9:35 am on September 26, 2023:
member
lgtm
fanquake marked this as ready for review
on Sep 26, 2023
sidhujag referenced this in commit
109e98bd01
on Sep 26, 2023
in
ci/test/00_setup_env_win64.sh:10
in
ef94d73804outdated
6@@ -7,13 +7,10 @@
7 export LC_ALL=C.UTF-8
8 9 export CONTAINER_NAME=ci_win64
10-export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:22.04" # Check that Jammy can cross-compile to win64
11+export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:23.10" # Check that https://packages.ubuntu.com/mantic/g++-mingw-w64-x86-64-posix (version 12.2, similar to guix) can cross-compile
maflcko
commented at 12:30 pm on September 29, 2023:
Just for the order in which to merge things, I’d prefer to keep this in sync with guix, so merging “https://github.com/bitcoin/bitcoin/pull/27897 (guix: use GCC 12.3.0 to build releases by fanquake)” first would be better.
Also, it would be good to stay on some LTS release to avoid having to touch CI every few months.
Maybe use the 24.04 tag, once it is available in a month or two?
fanquake
commented at 10:41 am on October 2, 2023:
Yea. Rebased this back on #27897, and moved this to draft for now.
fanquake force-pushed
on Oct 2, 2023
fanquake marked this as a draft
on Oct 2, 2023
Retropex referenced this in commit
32fc68d9b1
on Oct 4, 2023
fanquake force-pushed
on Oct 4, 2023
fanquake force-pushed
on Oct 13, 2023
fanquake force-pushed
on Oct 30, 2023
fanquake force-pushed
on Nov 3, 2023
fanquake force-pushed
on Nov 8, 2023
DrahtBot added the label
Needs rebase
on Nov 13, 2023
fanquake force-pushed
on Nov 13, 2023
DrahtBot removed the label
Needs rebase
on Nov 13, 2023
fanquake force-pushed
on Nov 16, 2023
fanquake force-pushed
on Nov 22, 2023
fanquake force-pushed
on Nov 28, 2023
div72 referenced this in commit
5c57147426
on Dec 3, 2023
fanquake force-pushed
on Dec 7, 2023
DrahtBot added the label
CI failed
on Dec 7, 2023
fanquake force-pushed
on Dec 8, 2023
maflcko
commented at 2:46 pm on December 8, 2023:
member
DrahtBot added the label
Needs rebase
on Dec 13, 2023
fanquake force-pushed
on Dec 22, 2023
DrahtBot removed the label
Needs rebase
on Dec 22, 2023
fanquake force-pushed
on Jan 15, 2024
fanquake force-pushed
on Feb 20, 2024
fanquake force-pushed
on Feb 28, 2024
maflcko
commented at 8:30 am on February 29, 2024:
member
Looks like the tidy task fails?
PastaPastaPasta referenced this in commit
cc8ea6484b
on Feb 29, 2024
PastaPastaPasta referenced this in commit
49a919f18a
on Feb 29, 2024
PastaPastaPasta referenced this in commit
b74e2b3a0e
on Feb 29, 2024
PastaPastaPasta referenced this in commit
023eb917a8
on Feb 29, 2024
PastaPastaPasta referenced this in commit
d31fe6c750
on Mar 3, 2024
PastaPastaPasta referenced this in commit
3b7f29ff6e
on Mar 4, 2024
PastaPastaPasta referenced this in commit
f9b526bd31
on Mar 5, 2024
fanquake force-pushed
on Mar 14, 2024
in
ci/test/00_setup_env_win64.sh:10
in
d72888c4d5outdated
6@@ -7,13 +7,10 @@
7 export LC_ALL=C.UTF-8
8 9 export CONTAINER_NAME=ci_win64
10-export CI_IMAGE_NAME_TAG="docker.io/amd64/debian:bookworm" # Check that https://packages.debian.org/bookworm/g++-mingw-w64-x86-64-posix (version 12.2, similar to guix) can cross-compile
11+export CI_IMAGE_NAME_TAG="docker.io/amd64/ubuntu:23.10" # Check that https://packages.ubuntu.com/mantic/g++-mingw-w64-x86-64-posix (version 12.3, similar to guix) can cross-compile
Will add it’s also a better match to Guix, as there we are using GCC 12.3.0 & 11.x headers, and that is what would be used on Mantic, vs GCC 12.2.0 and 10.x on Debian.
Is there a particular reason it can’t be used in the interim, so we have a better matching CI and less workarounds to maintain? I assume we are going to switch to Noble when it releases in April, so Mantic going EOL in July isn’t an issue?
Going to drop this for now, we can retain the mingw-w64 header workaround, and the smaller mismatch.
fanquake force-pushed
on Mar 14, 2024
fanquake
commented at 4:01 pm on March 14, 2024:
member
Dropped the Mantic bump, rebased on top of #29165, which should fix the failing multiprocess CI (Clang bump to 15), and added a commit to fix the tidy job (works around a bug in llvm 17, fixed in 18).
fanquake
commented at 11:05 am on March 15, 2024:
member
DrahtBot removed the label
DrahtBot Guix build requested
on Mar 19, 2024
fanquake force-pushed
on Mar 20, 2024
fanquake force-pushed
on Mar 26, 2024
fanquake force-pushed
on Apr 1, 2024
DrahtBot added the label
CI failed
on Apr 1, 2024
janus referenced this in commit
7bfb1b86db
on Apr 1, 2024
DrahtBot removed the label
CI failed
on Apr 5, 2024
fanquake force-pushed
on Apr 6, 2024
hebasto
commented at 5:48 pm on April 6, 2024:
member
Tested 8d2622f96f7bd25a4ab9c667bd00f374cfcc15a6.
The CI script ci/test/00_setup_env_native_valgrind.sh fail locally:
0wallet/walletdb.cpp:1431:15: error: code will never be executed [-Werror,-Wunreachable-code]
1 error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
2^ 3wallet/walletdb.cpp:1426:19: note: silence by adding parentheses to mark code as explicitly dead
4if constexpr (true) {
5^ 6/* DISABLES CODE */ ( )
7wallet/walletdb.cpp:1419:19: error: code will never be executed [-Werror,-Wunreachable-code]
8 error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", fs::PathToString(path)));
9^10wallet/walletdb.cpp:1414:23: note: silence by adding parentheses to mark code as explicitly dead
11if constexpr (true) {
12^13/* DISABLES CODE */ ( )
142 errors generated.
fanquake
commented at 5:58 pm on April 6, 2024:
member
The CI script ci/test/00_setup_env_native_valgrind.sh fail locally:
That is a bug in Clang 14. Fixed in 15.
fanquake force-pushed
on Apr 10, 2024
DrahtBot added the label
CI failed
on Apr 10, 2024
fanquake force-pushed
on Apr 14, 2024
DrahtBot removed the label
CI failed
on Apr 14, 2024
maflcko added the label
DrahtBot Guix build requested
on Apr 23, 2024
DrahtBot
commented at 4:36 pm on April 23, 2024:
contributor
DrahtBot removed the label
DrahtBot Guix build requested
on Apr 23, 2024
fanquake force-pushed
on Apr 25, 2024
DrahtBot added the label
CI failed
on Apr 26, 2024
DrahtBot
commented at 1:06 am on April 26, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
DrahtBot removed the label
CI failed
on Apr 26, 2024
fanquake force-pushed
on Apr 29, 2024
theuni
commented at 6:22 pm on May 1, 2024:
member
What’s the status of this?
fanquake
commented at 1:18 am on May 2, 2024:
member
It’s waiting for the PR linked in the description. Otherwise, should be mergable.
maflcko added the label
Needs CMake port
on May 2, 2024
fanquake force-pushed
on May 2, 2024
fanquake
commented at 1:26 pm on May 2, 2024:
member
This will likely fail one last time, but should be good after #30012.
fanquake force-pushed
on May 2, 2024
DrahtBot added the label
CI failed
on May 2, 2024
DrahtBot
commented at 2:05 pm on May 2, 2024:
contributor
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
fanquake marked this as ready for review
on May 2, 2024
DrahtBot removed the label
CI failed
on May 2, 2024
ci: disable -Werror=maybe-uninitialized for Windows builds
This produces false positives, such as:
```bash
torcontrol.cpp: In static member function ‘static void TorControlConnection::readcb(bufferevent*, void*)’:
torcontrol.cpp:94:28: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
94 | self->message.code = ToIntegral<int>(s.substr(0, 3)).value_or(0);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./netaddress.h:18,
from ./torcontrol.h:11,
from torcontrol.cpp:6:
./util/strencodings.h:184:7: note: ‘result’ was declared here
184 | T result;
| ^~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:11088: libbitcoin_node_a-torcontrol.o] Error 1
```
7469ac7032
ci: remove -Warray-bounds from -Werror for win64
These warnings are coming from leveldb, and appear to be fixed starting
with GCC 13.
bec6a88fbc
ci: remove -Wdocumentation from -Werror in multiprocess CI
The issues here are coming from Boost Test code.
b088062e68
build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set
Now that CXXFLAGS are back in user control, I don't think there's a
reason to no-longer use our warning flags when CXXFLAGS has been
overriden (this includes when building from depends).
Anyone can suppress warnings from third-party code by
passing the relevant `-Wno-` options in CXXFLAGS.
Fixes: #18092.
f0e22be69a
fanquake force-pushed
on May 3, 2024
fanquake added the label
DrahtBot Guix build requested
on May 3, 2024
maflcko
commented at 9:39 am on May 3, 2024:
member
Would it make sense to mention that -Wno-error=... should be passed, when needed, in the compile docs?
theuni
commented at 1:32 pm on May 3, 2024:
member
I can’t repro the -Wmaybe-uninitialized issue. Might it be possible to just add the initializations (even if they’re unnecessary) rather than disabling?
Like for the example provided, making it:
0T result{};
Edit: nm, I see there are other false-positives as well.
theuni approved
theuni
commented at 1:38 pm on May 3, 2024:
member
ACKf0e22be69a15248c42964d57f44ce8c37a36081d. It’ll be nice to have this fixed.
maflcko
commented at 1:43 pm on May 3, 2024:
member
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-12-22 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me