build: align debugging flags to -O0
#29796
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29796. ReviewsSee 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. ConflictsNo conflicts as of last run. |
Concept ACK on fixing that. |
|
Concept ACK |
|
Concept ACK. I think msan is a good proxy for what we want enabled. From its docs:
From gcc's docs on
From what I can tell, |
|
While this does resolve the compile issue I was having, it does seem to change how gdb is able to debug things, possibly in a meaningful way? This is just an example of a difference that I noticed while stepping through On master: This PR: One thing that jumps out to me as possibly being problematic, depending on what's being debugged, is that the line Another thing that is kind of annoying is the appearance of the |
|
@achow101 Yeah, I see the same. Sadly that's the trade-off. Basically So we have to make a choice: pure debug-ability (including things like inlines which don't actually exist), or more realistic binaries while sacrificing some source code in the debugger. Personally I prefer the latter, but you probably live in gdb more than me :) |
|
Note that for local hacking (with clang), you can use: diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 96c4397504..6bc408e843 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2944,7 +2944,7 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
return MakeDatabase(wallet_path, options, status, error_string);
}
-std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings)
+std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::string& name, std::unique_ptr<WalletDatabase> database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) __attribute__ ((optnone))
{
interfaces::Chain* chain = context.chain;
ArgsManager& args = *Assert(context.args);To get the best of both worlds: Thread 1 "b-test" hit Breakpoint 1, wallet::CWallet::Create (context=..., name="/tmp/test_common_Bitcoin Core/231b391a2079bb9568d880b8e884ab0b1c6a21c39361735a83653cf8256a247e/test_wallet_2950041828", database=std::unique_ptr<wallet::WalletDatabase> = {...}, wallet_creation_flags=25769803776, error=..., warnings=std::vector of length 0, capacity 0) at ./src/wallet/wallet.cpp:2949
2949 interfaces::Chain* chain = context.chain;
(gdb) n
2950 ArgsManager& args = *Assert(context.args);
(gdb)
2951 const std::string& walletFile = database->Filename();
(gdb)
2953 const auto start{SteadyClock::now()};
(gdb)
2956 std::shared_ptr<CWallet> walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet);
(gdb)
2957 walletInstance->m_keypool_size = std::max(args.GetIntArg("-keypool", DEFAULT_KEYPOOL_SIZE), int64_t{1});
(gdb)
2958 walletInstance->m_notify_tx_changed_script = args.GetArg("-walletnotify", "");
(gdb)
2961 bool rescan_required = false; |
|
One thing to investigate is if |
458216e to
38ab095
Compare
38ab095 to
d7f2024
Compare
d7f2024 to
be857c0
Compare
|
@theuni reminder that you might want to followup here at some point, re recent discussion/debugging. |
be857c0 to
58e1cb3
Compare
58e1cb3 to
a435069
Compare
There was a problem hiding this comment.
Code review ACK a435069. I don't know all of the tradeoffs between flags, but this change makes sense and seems to do what is described.
I did notice this is changing darwin, linux, and windows flags from O1 to Og but not freebsd/netbsd/openbsd flags so was curious if that was intentional.
a435069 to
296def1
Compare
It was not. Have fixed that now. Will take this out of draft. |
There was a problem hiding this comment.
Code review ACK 296def1.
However, reading more about this it seems like there are real examples of where -Og hurts debuggability #16435 (comment) ("Yes. Please! On macOS, without that fix, debugging with lldb/gdb is impossible.") and earlier in this thread.
Also if the goal is to make depends build and main build more consistent, I'm not sure why they couldn't both use -O0 or if the other problems mentioned with -O0 prevent that.
But overall it seems like an improvement to use -Og not -O1 in debug depends builds, and to experiment -Og it in the main build to be more consistent. However, If it turns out -Og does negatively impact debugging in the main build, I think we should consider breaking consistency and using -O0 again in the main build by default. We could also recommend manually switching to -O0 in developer notes when using GDB.
-O0-O0
7e5cf50 to
381ab3f
Compare
381ab3f to
d4e8743
Compare
d4e8743 to
6d33581
Compare
6d33581 to
07907e7
Compare
There was a problem hiding this comment.
Code review ACK 07907e7
Would be good to update the PR description to match current state of the PR and how it is changing depends debug flags from -O1 to -O0. Could also update the "I'm not completely sure yet what the right choice is" part to say what the problems were with other alternatives tried here.
|
Not sure about this. Going from O1 to O0 in the CI will make the CI tasks a lot slower. For example the multiprocess task is now taking almost two hours https://cirrus-ci.com/task/6440292238229504, whereas it never was that slow in the whole history of CI: https://0xb10c.github.io/bitcoin-core-ci-stats/graph/execution/#multiprocess,%20i686,%20DEBUG (the uppper outliers are this pull request) I understand that O0 can be used to detect more bugs, but when it comes to the CI, we have to realize that there aren't infinite resources and it will never be possible to put everything into the CI in this repo to run on every push to every pull request. There are already CI tasks that are run outside this repo and when it comes to O0, this is probably something to run externally as well. |
1ef22ce depends: patch around PlacementNew issue in capnp (fanquake) Pull request description: See #31772 and capnproto/capnproto#2235. Given there isn't agreement in #29796, pulled this out so it could be merged separately, and it's easier to run different test configurations externally. Closes #31772. ACKs for top commit: ryanofsky: Code review ACK 1ef22ce. Confirmed patch is identical to one merged upstream. Only change since last review was tweaking the file paths and commit data in the patch. TheCharlatan: ACK 1ef22ce Tree-SHA512: 9c9ecf50c43cf74315f6659afab55aeafb436f70e83b328016ad574136dce46867220c6e1a85aefd8d3d3d027cd94cc807c79721a4983da9428de70f11224e52
When compiled under -O0, this code was causing runtime failures in the 32-bit Clang gui wallet tests. Work around that by importing a commit from upstream: qt/qtbase@8c8b9a4.
07907e7 to
4bdc609
Compare
|
no opinion about the build changes here, but about the CI changes I slightly tend toward Approach NACK, as per my previous comment #29796 (comment) |
|
@fanquake Can you describe why you think it's important that they align? I think they're 2 different concerns, really. It seems like it'd be a pretty rare circumstance where we'd need depends compiled with max gdb debugability, but pretty common that we'd want Core built that way. Aligning depends and Core builds both at So my question is.. what does depends built with |
It can't hurt to have the option to have a stronger msan build for depends (see #22064 (comment)), but I don't know if it will be beneficial by default or in other scenarios. Edit: When it comes to the CI, we could even consider to set |
Mostly for more intuitive behaviour. I think this was more interesting when it was going to be -Og or -O1. Maybe the better approach is to discuss this (and the passing through of flags) further as part of #31920. I'll split out the Qt cherry-pick and close this PR. At least this has surfaced a couple bugs. |
|
Fwiw I do think it's interesting to test this somehow now and then to surface bugs like that. I'm just not convinced that |
A depends build with
DEBUG=1and usingcmake -B build -DCMAKE_BUILD_TYPE=Debugdo not align, which is inconsistent/confusing. The depends build will use-O1, whileCMAKE_BUILD_TYPE=Debugwill set-O0.I'm not completely sure yet what the right choice is (previously it seems that
-Ogwas unusable with Clang? (note that for Clang it looks like-Ogand-O1are basically equivalent), but we should at least align the two systems to be using the same thing for debugging.Configure history:
-O0added in #3833Switched to
-Ogin #13005.Switched back to
-O0#16435Depends
DEBUG=1mode has used-O1since inception.