[RFC] Switch and/or align debugging flags (back) to -Og
#29796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
Concept ACK on fixing that. |
|
Concept ACK |
1 task
|
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 |
fanquake
force-pushed
the
depends_0g_debug_flags
branch
from
458216e
to
38ab095
Compare
fanquake
force-pushed
the
depends_0g_debug_flags
branch
from
38ab095
to
d7f2024
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently have two issues in relation to debugging optimisation flags:
DEBUG=1and using./configure --enable-debugdo not align, which is inconsistent/confusing. Depends currently uses-O1, while--enable-debugwill set-O0.-O0is unusable in various ways:-O0doesn't currently work: build: remove confusing and inconsistent disable-asm option #29407 (comment) (probably an upstream issue?).-O0is broken without additional flags: build: Windows debug cross-build fails with-O0#28109.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.