Do not evaluate hidden LogPrint arguments #9417
Merged
Conversation
This file contains hidden or 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
Contributor
|
@sipa Thanks - I was just about to look into coding something similar so you have saved me the job. In the similar way that GetBoolArg() works, I wondered if perhaps we could have a boolean check for whether various debug are set, or perhaps even a scenario where we can say log this if debug=tx OR debug=mempool is set since I have some debug lines from mempool and tx functions merged, perhaps something like Looking at the code it looks like the boolean function exists already, but the 2nd part (the "this category or that category" would still be nice). |
Contributor
Contributor
|
ACK. Additional benefit: if evaluation of an argument would crash the software, e.g. via evaluating vector[0] without checking the size, that won't happen if the debugging is not actually turned on. Not all side effects are good. :) |
Member
|
Makes sense. It's unfortunate that this needs a macro solution.
utACK 407cdd6 |
laanwj
added a commit
that referenced
this pull request
Jan 5, 2017
407cdd6 Do not evaluate hidden LogPrint arguments (Pieter Wuille)
Member
Author
|
I looked this up. I find mentions that C++11 mandates support for vararg
macros, however, it's not even listed in GCC's C++11 support pages. Perhaps
it's been supported for so long that they didn't bother listing it? Then I
noticed we're already using vararg templates in the codebase, so I stopped
worrying.
|
sickpig
referenced
this pull request
in sickpig/BitcoinUnlimited
Jan 11, 2018
407cdd6 Do not evaluate hidden LogPrint arguments (Pieter Wuille)
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Jan 18, 2018
407cdd6 Do not evaluate hidden LogPrint arguments (Pieter Wuille)
andvgal
pushed a commit
to energicryptocurrency/gen2-energi
that referenced
this pull request
Jan 6, 2019
407cdd6 Do not evaluate hidden LogPrint arguments (Pieter Wuille)
CryptoCentric
pushed a commit
to absolute-community/absolute
that referenced
this pull request
Feb 26, 2019
407cdd6 Do not evaluate hidden LogPrint arguments (Pieter Wuille)
furszy
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Mar 27, 2020
9fb0a43 util: Throw tinyformat::format_error on formatting error (random-zebra) 67eb699 util: Properly handle errors during log message formatting (random-zebra) 66ec97b Do not evaluate hidden LogPrint arguments (random-zebra) 2713458 util: Remove zero-argument versions of LogPrint and error (random-zebra) 500dfee util: Update tinyformat (random-zebra) 6837887 util: switch LogPrint and error to variadic templates (random-zebra) 0fa578f tinyformat: force USE_VARIADIC_TEMPLATES (random-zebra) Pull request description: this backports the following pull requests from upstream bitcoin: - bitcoin#8000 (0fa578f, 6837887) > Now that we started using c++11, force use of variadic templates. The autodetection may be wonky on some compilers, see discussion here and is unnecessary for us anyhow. - bitcoin#8274 (500dfee, 2713458) > Updates tinyformat.h to commit c42f/tinyformat@3a33bbf upstream. - bitcoin#9417 (66ec97b) > There are a few cases where hashes are computed inside LogPrint arguments - where they usually go unused. As LogPrint statements should never have side effects besides printing something, we can avoid the evaluation in this case. Advantage: perhaps a small performance improvement; I haven't benchmarked. Disadvantage: if we would have statements with side-effects, this could make this a bit harder to debug. - bitcoin#9963 (67eb699, 9fb0a43) > Instead of having an exception propagate into the program (which at worst causes a crash) when an error happens while formatting a log message, just print a message to the log. This message clearly indicates what log message was formatted wrongly, and what error happened during formatting it. ACKs for top commit: Fuzzbawls: ACK 9fb0a43 furszy: cool, utACK 9fb0a43 . Tree-SHA512: 29a902bb712612ca093a8fe863e9eff1c17d40955bbc37a5ceb6f057068ac6150dfcffd6836a6b0bd6295aa9bffd0925eb69e50f82ccaa7d84215efc274a59a4
akshaynexus
pushed a commit
to ZENZO-Ecosystem/ZENZO-Core
that referenced
this pull request
Mar 30, 2020
9fb0a43 util: Throw tinyformat::format_error on formatting error (random-zebra) 67eb699 util: Properly handle errors during log message formatting (random-zebra) 66ec97b Do not evaluate hidden LogPrint arguments (random-zebra) 2713458 util: Remove zero-argument versions of LogPrint and error (random-zebra) 500dfee util: Update tinyformat (random-zebra) 6837887 util: switch LogPrint and error to variadic templates (random-zebra) 0fa578f tinyformat: force USE_VARIADIC_TEMPLATES (random-zebra) Pull request description: this backports the following pull requests from upstream bitcoin: - bitcoin#8000 (0fa578f, 6837887) > Now that we started using c++11, force use of variadic templates. The autodetection may be wonky on some compilers, see discussion here and is unnecessary for us anyhow. - bitcoin#8274 (500dfee, 2713458) > Updates tinyformat.h to commit c42f/tinyformat@3a33bbf upstream. - bitcoin#9417 (66ec97b) > There are a few cases where hashes are computed inside LogPrint arguments - where they usually go unused. As LogPrint statements should never have side effects besides printing something, we can avoid the evaluation in this case. Advantage: perhaps a small performance improvement; I haven't benchmarked. Disadvantage: if we would have statements with side-effects, this could make this a bit harder to debug. - bitcoin#9963 (67eb699, 9fb0a43) > Instead of having an exception propagate into the program (which at worst causes a crash) when an error happens while formatting a log message, just print a message to the log. This message clearly indicates what log message was formatted wrongly, and what error happened during formatting it. ACKs for top commit: Fuzzbawls: ACK 9fb0a43 furszy: cool, utACK 9fb0a43 . Tree-SHA512: 29a902bb712612ca093a8fe863e9eff1c17d40955bbc37a5ceb6f057068ac6150dfcffd6836a6b0bd6295aa9bffd0925eb69e50f82ccaa7d84215efc274a59a4
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
There are a few cases where hashes are computed inside LogPrint arguments - where they usually go unused. As LogPrint statements should never have side effects besides printing something, we can avoid the evaluation in this case.
Advantage: perhaps a small performance improvement; I haven't benchmarked.
Disadvantage: if we would have statements with side-effects, this could make this a bit harder to debug.