This supports lcov 2.x in the sense that we are no-longer hardcoding
version specific options, and instead will use the `LCOV_OPTS` variable
(which is the more correct/flexible thing to do in any case). It's also
quite likely that devs are already having to pass extra options to lcov
2.x, given it's more stringent in terms of coverage generation and error
checking. See this thread for an example:
https://github.com/linux-test-project/lcov/issues/238.
Added an example to the developer notes.
Tested on one machine (LCOV 2.0, gcc 13.2) with:
```bash
./autogen.sh
./configure --enable-lcov CXXFLAGS="-fprofile-update=prefer-atomic" LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch"
make
make cov
<snip>
Processing file src/netaddress.cpp
lines=521 hit=480 functions=72 hit=72 branches=675 hit=499
Overall coverage rate:
lines......: 81.8% (79362 of 97002 lines)
functions......: 77.8% (10356 of 13310 functions)
branches......: 49.6% (130628 of 263196 branches)
```
and another machine (LCOV 2.1, Clang 18.1.3) with:
```bash
./autogen.sh
./configure --enable-lcov CC=clang CXX=clang++ LCOV_OPTS="--rc branch_coverage=1 --ignore-errors mismatch,inconsistent"
make
make cov
<snip>
Processing file src/util/strencodings.cpp
lines=315 hit=311 functions=38 hit=38 branches=425 hit=357
Overall coverage rate:
source files: 622
lines.......: 79.8% (70311 of 88132 lines)
functions...: 78.1% (13968 of 17881 functions)
branches....: 44.5% (157551 of 354317 branches)
Message summary:
101 warning messages:
count: 1
inconsistent: 100
3528 ignore messages:
inconsistent: 3528
```
e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e5b70327e019382320f5ef0a651ecc5 logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a05663ad7a53908e910d0f42b49b881e09 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e3297645527b07314e1d5a82275fb25845 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012351e7b8aab871dd9d2b38ade1cd9bc logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874da04e45f68d17575c1e8a5431ca9bc logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615677d226c9f6b3f2b66d833315d40da logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
achow101:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
stickies-v:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c
jamesob:
ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
91d08889218e06631f43a3dab0bae576aa46e43c sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov)
3df37e0c78c3d5139c963a74eda56c331355ef72 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov)
Pull request description:
Constructs like
```cpp
AssertLockNotHeld(m);
LOCK(m);
```
are equivalent to (almost, modulo some logging differences, see below)
```cpp
LOCK(m);
```
for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in such cases. Requests to do the former keep coming during review process. `developer-notes.md` explicitly states "Combine annotations in function declarations with run-time asserts in function definitions", but that seems to be too strong or unclear. `LOCK()` is also a run-time assert in this case.
Also remove `LocksHeld()` from the public interface in `sync.h` since it is only used in `sync.cpp`.
ACKs for top commit:
achow101:
ACK 91d08889218e06631f43a3dab0bae576aa46e43c
hebasto:
ACK 91d08889218e06631f43a3dab0bae576aa46e43c, I have reviewed the code and it looks OK.
Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.
Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.
-BEGIN VERIFY SCRIPT-
sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)
-END VERIFY SCRIPT-
da347de530242ead8f6a9718ee1440385bd3d44d doc: update broken links (pablomartin4btc)
Pull request description:
References to `utilstrencodings` and `lint-locale-dependence.sh` where incorrect, updating them accordingly.
Also, adding another reference to util function [`LocaleIndependentAtoi`](https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L108-L118), which is related with the updated section of the guide:
```
// LocaleIndependentAtoi is provided for backwards compatibility reasons.
//
// New code should use ToIntegral or the ParseInt* functions
// which provide parse error feedback.
//
// The goal of LocaleIndependentAtoi is to replicate the defined behaviour of
// std::atoi as it behaves under the "C" locale, and remove some undefined
// behavior. If the parsed value is bigger than the integer type's maximum
// value, or smaller than the integer type's minimum value, std::atoi has
// undefined behavior, while this function returns the maximum or minimum
// values, respectively.
```
ACKs for top commit:
MarcoFalke:
lgtm ACK da347de530242ead8f6a9718ee1440385bd3d44d
Tree-SHA512: c8f4cd9cff1fb3ea367ac9dbe5aa45dc187fc60114f2e2106e02e0e17fea4ee34d6e0c408fe920c2d8765e06b4dc30c231f0454fa35469c4399e0cadbcd341ba
Constructs like
```cpp
AssertLockNotHeld(m);
LOCK(m);
```
are equivalent to
```cpp
LOCK(m);
```
for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in
such cases.
75347236f212f327a5bba10d8a900cc58ebe5de0 docs: document c-style cast prohibition (Pasta)
Pull request description:
In the words of practicalswift:
```
A C-style cast is equivalent to try casting in the following order:
const_cast(...)
static_cast(...)
const_cast(static_cast(...))
reinterpret_cast(...)
const_cast(reinterpret_cast(...))
By using static_cast<T>(...) explicitly we avoid the possibility of an unintentional and
dangerous reinterpret_cast. Furthermore static_cast<T>(...) allows for easier grepping of casts.
For a more thorough discussion, see "ES.49: If you must use a cast, use a named cast"
in the C++ Core Guidelines (Stroustrup & Sutter).
```
Modern tooling, specifically `-Wold-style-cast` can enable us to enforce never using C-style casts. I believe this is especially important due to the number of C-style casts the codebase is currently being used as a reinterpret_cast. reinterpret_casts are especially dangerous, and should never be done via C-style casts.
Update the docs to suggest the use of named cast or functional casts.
Top commit has no ACKs.
Tree-SHA512: 29a98de396f0c78e32d8a1831319162203c4405a670da5add5da956fcc7df200a1cec162ef1cfac4ddfb02714b66406081d40ed435c7f0f28581cfa24d94fac1
Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns)
d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37e1b2d19e5a053dbfefa30306c1d41a net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
hebasto:
ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
that was added in 2015 by commit b8c06ef40 in PR 7003, as that potential issue
would now be caught by the test/lint/lint-format-strings.py script run by the CI
52a797bfe5ced4329f2272be417c35730ec8839f doc: Avoid ADL for function calls (Hennadii Stepanov)
Pull request description:
It happened two times recently, when [ADL](https://en.cppreference.com/w/cpp/language/adl) popped up unexpectedly and brought some confusion:
- https://github.com/bitcoin/bitcoin/pull/24338/files#r805989994
> Any idea why this even compiles?
- https://www.erisian.com.au/bitcoin-core-dev/log-2022-02-18.html#l-51:
> 2022-02-18T03:24:14 \<dongcarl\> Does anyone know why this compiles? 6d3d2caa37
> 2022-02-18T03:24:14 \<dongcarl\> GetUTXOStatsWithHasher and MakeUTXOHasher are both in the `kernel::` namespace and I never added a `using` declaration on top...
> 2022-02-18T03:25:53 \<sipa\> https://en.cppreference.com/w/cpp/language/adl ?
Let's document our intention to avoid similar cases in the future.
ACKs for top commit:
laanwj:
Anyhow, ACK 52a797bfe5ced4329f2272be417c35730ec8839f, there is no need to hold merge up on this, documenting it is a step forward.
Tree-SHA512: f52688b5d8f6130302185206ec6ea4731b099a75294ea2d477901a52d6d58473e3427e658aea408c140c2824c37a0399ec7376aded2a91197895ea52d51f0018
0bc666b053b8f4883c3f5de43959e2bbd91b95c5 doc: add info for debugging with relative paths (S3RK)
a8b515c317f0b5560f62c72a8f4eb6560d8f1c75 configure: keep relative paths in debug info (S3RK)
Pull request description:
This is a follow-up for #20353 that fixes#21885
It also adds a small section to assist debugging without absolute paths in debug info.
ACKs for top commit:
kallewoof:
Tested ACK 0bc666b053b8f4883c3f5de43959e2bbd91b95c5
Zero-1729:
Light crACK 0bc666b053b8f4883c3f5de43959e2bbd91b95c5
Tree-SHA512: d4b75183c3d3a0f59fe786841fb230581de87f6fe04cf7224e4b89c520d45513ba729d4ad8c0e62dd1dbaaa7a25741f04d036bc047f92842e76c9cc31ea47fb2
