doc: Document how to analyze Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck #18920

pull practicalswift wants to merge 3 commits into bitcoin:master from practicalswift:static-analysis-clang-tidy changing 1 files +55 −0

practicalswift commented at 10:17 AM on May 9, 2020: contributor

Document how to analyze Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck.

As requested by Sjors (#14676) and others :)
fanquake added the label Docs on May 9, 2020
practicalswift renamed this:
~~doc: Document how to analyze Bitcoin Core using clang-tidy~~
doc: Document how to analyze Bitcoin Core using clang-tidy and cppcheck
on May 9, 2020
practicalswift force-pushed on May 9, 2020
doc: Document how to analyze Bitcoin Core using clang-tidy d51e0c01c4
doc: Document how to analyze Bitcoin Core using cppcheck 03c5cb61bb
doc: Document how to analyze Bitcoin Core using Clang Static Analyzer bbfbdcf2ba
practicalswift force-pushed on May 9, 2020
practicalswift renamed this:
~~doc: Document how to analyze Bitcoin Core using clang-tidy and cppcheck~~
doc: Document how to analyze Bitcoin Core using Clang Static Analysis, clang-tidy and cppcheck
on May 9, 2020

in doc/analysis.md:13 in bbfbdcf2ba

   8 | +
   9 | +```
  10 | +$ git clone https://github.com/bitcoin/bitcoin
  11 | +$ cd bitcoin/
  12 | +$ ./autogen.sh
  13 | +$ CC=clang CXX=clang++ ./configure --with-incompatible-bdb --disable-ccache

jonatack commented at 12:18 PM on May 9, 2020:

It may be helpful to mention why ccache needs to be disabled.

jonatack commented at 12:21 PM on May 9, 2020: member

Concept ACK. Good idea. I think each guide could begin with a sentence on why/what and not just how. Could squash to one commit.

in doc/analysis.md:52 in bbfbdcf2ba

  47 | +$ make
  48 | +$ cd ..
  49 | +# Analyze source code files ...
  50 | +$ cppcheck/bin/cppcheck --language=c++ -D__cplusplus -DCLIENT_VERSION_BUILD \
  51 | +    -DCLIENT_VERSION_IS_RELEASE -DCLIENT_VERSION_MAJOR -DCLIENT_VERSION_MINOR \
  52 | +    -DCLIENT_VERSION_REVISION -DCOPYRIGHT_YEAR -DDEBUG

jonatack commented at 12:27 PM on May 9, 2020:

missing backslash at EOL

jonatack commented at 12:31 PM on May 9, 2020:

heh, nice example!

$ cppcheck/bin/cppcheck --language=c++ -D__cplusplus .../... src/net_processing.cpp
src/net_processing.cpp:936:17: error: Same iterator is used with different containers 'mapOrphanTransactions' and 'itPrev.second'. [iterators1]
        itPrev->second.erase(it);
                ^
src/limitedmap.h:72:39: style: Same iterators expression are used for algorithm. [sameIteratorExpression]
        iterator itTarget = map.erase(itIn, itIn);

in doc/analysis.md:14 in bbfbdcf2ba
```
   9 | +```
  10 | +$ git clone https://github.com/bitcoin/bitcoin
  11 | +$ cd bitcoin/
  12 | +$ ./autogen.sh
  13 | +$ CC=clang CXX=clang++ ./configure --with-incompatible-bdb --disable-ccache
  14 | +$ scan-build --use-cc=clang --use-c++=clang++ make
```
jonatack commented at 1:06 PM on May 9, 2020:

If this is OS-specific, I think it's necessary to state that. It seems to be, from looking at https://clang-analyzer.llvm.org/ and https://clang-analyzer.llvm.org/scan-build.html after seeing scan-build: 'ccc-analyzer' does not exist at '/usr/local/bin/ccc-analyzer' here and not finding a linux version.

in doc/analysis.md:34 in bbfbdcf2ba

  29 | +$ make all
  30 | +$ chmod +x bear/bear
  31 | +$ cd ..
  32 | +$ bear/bear/bear -l $(pwd)/bear/libear/libear.so make
  33 | +# Analyze source code files ...
  34 | +$ clang-tidy src/test/crypto_tests.cpp

jonatack commented at 1:44 PM on May 9, 2020:

Is this also OS-specific?

fanquake commented at 12:55 PM on May 19, 2020: member

I think I'd rather see this kind of documentation added to the devwiki (happy to help with that). That way it can be maintained / updated easier, without having to add even more documentation to this repo. There's also already other similar guides available, i.e I have one here: https://github.com/fanquake/core-review/blob/master/clang-tools.md.
MarcoFalke commented at 1:07 PM on May 19, 2020: member

There is also bitcoin-core/docs, if the devwiki or fanquake/core-review are insufficient places to put this.

In general I agree with @fanquake that documentation to compile with any imaginable compiler or checker or sanitizer or ... should probably be maintained outside of this repository. The project here is too big and moving too slow to keep every single piece of documentation up-to-date at all times. Also, we should be considerate of the precious review resource in this repository and use it on Bitcoin Core itself and not on integration of Bitcoin Core with meta developer tools.
practicalswift closed this on May 19, 2020
practicalswift commented at 2:03 PM on May 19, 2020: contributor

@fanquake

Feel free to take what you need from this PR and move to the appropriate place. Closing this PR :) @marcofalke

In general I agree with @fanquake that documentation to compile with any imaginable compiler or checker or sanitizer or ... should probably be maintained outside of this repository. The project here is too big and moving too slow to keep every single piece of documentation up-to-date at all times. Also, we should be considerate of the precious review resource in this repository and use it on Bitcoin Core itself and not on integration of Bitcoin Core with meta developer tools.

Personally I think we as a project have historically vastly underused the available tooling that is typically used in security critical projects to tame C++ (sanitizers, etc.). In other words I don't think we run the risk of using, integrating or documenting "too much" tooling (quite the opposite TBH!), but I see your point regarding moving the documentation to another repo :)
MarcoFalke commented at 2:17 PM on May 19, 2020: member

If one of the tools turns out to be useful for the greater project (for example be an optional but recommended tool for developers), I think it can surely be added to the documentation here.
jonatack commented at 2:33 PM on May 19, 2020: member

Feel free to take what you need from this PR and move to the appropriate place. Closing this PR :)

Thanks for this info @practicalswift. Like @fanquake, I've been compiling info like this in a separate repository for my own reference and anyone else who may find it helpful.
jonatack commented at 2:36 PM on May 19, 2020: member

similar guides available, i.e I have one here: fanquake/core-review:clang-tools.md@master .

Thanks @fanquake for the reminder to spend more time looking at the excellent info in core-review.
MarcoFalke referenced this in commit f13e03cda2 on Jan 7, 2021
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#14676 c++ linter instructions #20584 Declare de facto const reference variables/member functions as const