diff --git a/doc/developer-notes.md b/doc/developer-notes.md --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -209,14 +209,11 @@ ### Compiling for debugging -Run configure with `--enable-debug` to add additional compiler flags that -produce better debugging builds. +Run `cmake` with `-DCMAKE_BUILD_TYPE=Debug` to add additional compiler flags +that produce better debugging builds. ### Compiling for gprof profiling -Run configure with the `--enable-gprof` option, then make. - -With `cmake` and `ninja`: ``` cmake -GNinja .. -DENABLE_HARDENING=OFF -DENABLE_PROFIILING=gprof ``` @@ -244,7 +241,7 @@ 1. Uncomment the line with `#define UPDATE_JSON_TESTS` 2. Add a new TestBuilder to the `script_build` test to cover your test case. -3. `make && ./src/test/test_bitcoin --run_test=script_tests` +3. `ninja check-bitcoin-script_tests` 4. Copy your newly generated test JSON from `/src/script_tests.json.gen` to `src/test/data/script_tests.json`. Please commit your TestBuilder along with your generated test JSON and cleanup the uncommented #define before code review. @@ -261,10 +258,10 @@ ### DEBUG_LOCKORDER Bitcoin ABC is a multi-threaded application, and deadlocks or other -multi-threading bugs can be very difficult to track down. The `--enable-debug` -configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts -run-time checks to keep track of which locks are held, and adds warnings to the -debug.log file if inconsistencies are detected. +multi-threading bugs can be very difficult to track down. +The `-DCMAKE_BUILD_TYPE=Debug` cmake option adds `-DDEBUG_LOCKORDER` to the +compiler flags. This inserts run-time checks to keep track of which locks are +held, and adds warnings to the debug.log file if inconsistencies are detected. ### Valgrind suppressions file @@ -311,7 +308,7 @@ Bitcoin ABC can be compiled with various "sanitizers" enabled, which add instrumentation for issues regarding things like memory safety, thread race conditions, or undefined behavior. This is controlled with the -`--with-sanitizers` configure flag, which should be a comma separated list of +`-DENABLE_SANITIZERS` cmake flag, which should be a semicolon separated list of sanitizers to enable. The sanitizer list should correspond to supported `-fsanitize=` options in your compiler. These sanitizers have runtime overhead, so they are most useful when testing changes or producing debugging builds. @@ -320,32 +317,87 @@ ```bash # Enable both the address sanitizer and the undefined behavior sanitizer -./configure --with-sanitizers=address,undefined +cmake -GNinja .. -DENABLE_SANITIZERS="address;undefined" # Enable the thread sanitizer -./configure --with-sanitizers=thread +cmake -GNinja .. -DENABLE_SANITIZERS=thread ``` If you are compiling with GCC you will typically need to install corresponding "san" libraries to actually compile with these flags, e.g. libasan for the address sanitizer, libtsan for the thread sanitizer, and libubsan for the -undefined sanitizer. If you are missing required libraries, the configure script -will fail with a linker error when testing the sanitizer flags. +undefined sanitizer. If you are missing required libraries, the cmake script +will fail with an error when testing the sanitizer flags. + +Note that the sanitizers will give a better output if they are run with a Debug +build configuration. -The test suite should pass cleanly with the `thread` and `undefined` sanitizers, -but there are a number of known problems when using the `address` sanitizer. The -address sanitizer is known to fail in +There are a number of known problems for which suppressions files are provided +under `test/sanitizer_suppressions`. These files are intended to be used with +the `suppressions` option from the sanitizers. + +The address sanitizer is known to fail in [sha256_sse4::Transform](/src/crypto/sha256_sse4.cpp) which makes it unusable -unless you also use `--disable-asm` when running configure. We would like to fix -sanitizer issues, so please send pull requests if you can fix any errors found -by the address sanitizer (or any other sanitizer). +unless you also use `-DCRYPTO_USE_ASM=OFF` when running cmake. +We would like to fix sanitizer issues, so please send pull requests if you can +fix any errors found by the address sanitizer (or any other sanitizer). Not all sanitizer options can be enabled at the same time, e.g. trying to build -with `--with-sanitizers=address,thread` will fail in the configure script as +with `-DENABLE_SANITIZERS=="address;thread" will fail in the cmake script as these sanitizers are mutually incompatible. Refer to your compiler manual to learn more about these options and which sanitizers are supported by your compiler. +Examples: + +Build and run the test suite with the address sanitizer enabled: + +```bash +mkdir build_asan +cd build_asan + +export ASAN_OPTIONS="malloc_context_size=0" +export LSAN_OPTIONS="suppressions=${PWD}/../test/sanitizer_suppressions/lsan" + +cmake -GNinja .. \ + -DCMAKE_BUILD_TYPE=Debug \ + -DENABLE_SANITIZERS=address \ + -DCRYPTO_USE_ASM=OFF + +ninja check check-functional +``` + +Build and run the test suite with the thread sanitizer enabled (it can take a +very long time to complete): + +```bash +mkdir build_tsan +cd build_tsan + +export TSAN_OPTIONS="suppressions=${PWD}/../test/sanitizer_suppressions/tsan" + +cmake -GNinja .. \ + -DCMAKE_BUILD_TYPE=Debug \ + -DENABLE_SANITIZERS=thread + +ninja check check-functional +``` + +Build and run the test suite with the undefined sanitizer enabled: + +```bash +mkdir build_ubsan +cd build_ubsan + +export UBSAN_OPTIONS="suppressions=${PWD}/../test/sanitizer_suppressions/ubsan" + +cmake -GNinja .. \ + -DCMAKE_BUILD_TYPE=Debug \ + -DENABLE_SANITIZERS=undefined + +ninja check check-functional +``` + Additional resources: * [AddressSanitizer](https://clang.llvm.org/docs/AddressSanitizer.html) @@ -366,8 +418,8 @@ Deadlocks due to inconsistent lock ordering (thread 1 locks `cs_main` and then `cs_wallet`, while thread 2 locks them in the opposite order: result, deadlock as each waits for the other to release its lock) are a problem. Compile with -`-DDEBUG_LOCKORDER` (or use `--enable-debug`) to get lock order inconsistencies -reported in the debug.log file. +`-DDEBUG_LOCKORDER` (or use `-DCMAKE_BUILD_TYPE=Debug`) to get lock order +inconsistencies reported in the debug.log file. Re-architecting the core code so there are better-defined interfaces between the various components is a goal, with any necessary locking @@ -407,7 +459,7 @@ In closed-source environments in which everyone uses the same IDE it is common to add temporary files it produces to the project-wide `.gitignore` file. -However, in open source software such as Bitcoin Core, where everyone uses +However, in open source software such as Bitcoin ABC, where everyone uses their own editors/IDE/tools, it is less common. Only you know what files your editor produces and this may change from version to version. The canonical way to do this is thus to create your local gitignore. Add this to `~/.gitconfig`: @@ -420,7 +472,7 @@ (alternatively, type the command `git config --global core.excludesfile ~/.gitignore_global` on a terminal) -Then put your favourite tool's temporary filenames in that file, e.g. +Then put your favorite tool's temporary filenames in that file, e.g. ``` # NetBeans nbproject/ @@ -437,7 +489,7 @@ ============================ A few non-style-related recommendations for developers, as well as points to -pay attention to for reviewers of Bitcoin Core code. +pay attention to for reviewers of Bitcoin ABC code. Wallet ------- @@ -577,7 +629,7 @@ - Build and run tests with `-DDEBUG_LOCKORDER` to verify that no potential deadlocks are introduced. As of 0.12, this is defined by default when - configuring with `--enable-debug` + configuring with `-DCMAKE_BUILD_TYPE=Debug` - When using `LOCK`/`TRY_LOCK` be aware that the lock exists in the context of the current scope, so surround the statement and the code that needs the lock @@ -782,7 +834,7 @@ For example, if LevelDB had a bug that accidentally prevented a key from being returned in an edge case, and that bug was fixed upstream, the bug "fix" would be an incompatible consensus change. In this situation the correct behavior -would be to revert the upstream fix before applying the updates to Bitcoin's +would be to revert the upstream fix before applying the updates to Bitcoin ABC's copy of LevelDB. In general you should be wary of any upstream changes affecting what data is returned from LevelDB queries.