Add clang_static_analysis.py to help automate static analysis runs #9632

pull isle2983 wants to merge 1 commits into bitcoin:master from isle2983:PR-clang-static changing 2 files +372 −0
  1. isle2983 commented at 9:53 PM on January 25, 2017: contributor

    In the same vein as copyright_header.py (#9459) and basic_style.py (#9603), this script provides a consistent way to run clang static analysis checking on the codebase.

    $ contrib/devtools/clang_static_analysis.py -h
    usage: clang_static_analysis.py [-h] [-b BIN_PATH] [-r REPORT_PATH] [-j JOBS]
                                    {report,check} repository
    
    A utility for running clang static analysis on the codebase in a consistent
    way.
    
    positional arguments:
      {report,check}        Selects the output behavior. 'report' generates a
                            summary report on the issues found. 'check' compares
                            the state of the repository against a standard,
                            provides a return code for the shell and prints more
                            specific details when issues are found.
      repository            A source code repository for which the static analysis
                            is to be performed upon.
    
    optional arguments:
      -h, --help            show this help message and exit
      -b BIN_PATH, --bin-path BIN_PATH
                            The path holding 'scan-build' and 'scan-view'
                            binaries. (Uses 'scan-build' and 'scan-view' installed
                            in PATH by default)
      -r REPORT_PATH, --report-path REPORT_PATH
                            The path for scan-build to write its report files.
                            (default=/tmp/bitcoin-scan-build/)
      -j JOBS, --jobs JOBS  The number of parallel jobs to run with 'make'.
                            (default=6)
    

    The report subcommand is for providing a summary of the state of the checked-out branch. On current master, the output looks like this:

    $ contrib/devtools/clang_static_analysis.py report .
    Running command:     make clean
    Running command:     /usr/share/clang/scan-build-3.5/scan-build -k -plist-html --keep-empty -o /tmp/bitcoin-scan-build/ make -j6
    stderr/stdout to:    scan_build.log
    This might take a few minutes...
    Done.
    Results in:          /tmp/bitcoin-scan-build/2017-01-25-140103-6556-1
    
    --------------------------------------------------------------------------------
    Took 143.30 seconds to analyze with scan-build
    Found 3 issues:
    --------------------------------------------------------------------------------
    0: bench/bench.cpp:97:52 - Division by zero
    1: qt/paymentrequest.pb.cc:51:24 - Called C++ object pointer is null
    2: ./src/ecmult_gen_impl.h:153:5 - Value stored to 'bits' is never read
    --------------------------------------------------------------------------------
    Full details can be seen in a browser by running:
        $ /usr/share/clang/scan-view-3.5/scan-view /tmp/bitcoin-scan-build/2017-01-25-140103-6556-1
    --------------------------------------------------------------------------------
    

    The results directory is kept around in the /tmp/ location and scan-view gives you a full, very nice visualization of the results in your browser.

    The check subcommand provides a bit more specific detail and returns a shell status of zero only if there aren't any issues. Like with the other scripts, this is useful for eventual inclusion as a criteria for CI acceptance. Its output looks like this:

    $ contrib/devtools/clang_static_analysis.py check .
    Running command:     make clean
    Running command:     /usr/share/clang/scan-build-3.5/scan-build -k -plist-html --keep-empty -o /tmp/bitcoin-scan-build/ make -j6
    stderr/stdout to:    scan_build.log
    This might take a few minutes...
    Done.
    Results in:          /tmp/bitcoin-scan-build/2017-01-25-140349-11169-1
    
    --------------------------------------------------------------------------------
    An issue has been found in bench/bench.cpp:97:52
    Type:         Division by zero
    Description:  Division by zero
    
    0: bench/bench.cpp:93:5 - The value 0 is assigned to field 'count'
    1: bench/bench.cpp:97:52 - Division by zero
    --------------------------------------------------------------------------------
    An issue has been found in ./src/ecmult_gen_impl.h:153:5
    Type:         Dead assignment
    Description:  Value stored to 'bits' is never read
    
    0: ./src/ecmult_gen_impl.h:153:5 - Value stored to 'bits' is never read
    --------------------------------------------------------------------------------
    An issue has been found in qt/paymentrequest.pb.cc:51:24
    Type:         Called C++ object pointer is null
    Description:  Called C++ object pointer is null
    
    0: qt/paymentrequest.pb.cc:47:3 - 'file' initialized here
    1: qt/paymentrequest.pb.cc:50:3 - Assuming pointer value is null
    2: qt/paymentrequest.pb.cc:51:24 - Called C++ object pointer is null
    --------------------------------------------------------------------------------
    Full details can be seen in a browser by running:
        $ /usr/share/clang/scan-view-3.5/scan-view /tmp/bitcoin-scan-build/2017-01-25-140349-11169-1
    --------------------------------------------------------------------------------
    *** Static analysis issues found!
    

    Tthe first issue is fixed by PR #9547, the second looks like a trivial fix in secp256k1, and the third is a clear false-positive that is fixed in clang versions 3.9.0+. This output is from a run with clang 3.5.0 and all versions I tried (all of 3.4.0 through 3.9.1) return the same two real issues and no additional false-positives.

    There are additional, stricter plugins for clang's checking but this script doesn't invoke them. It is sticking to only the defaults for now. Presumably false-positives become more frequent as the strictness increases. Seeing as there doesn't appear to be anything too strange at this baseline level of checking, eventual inclusion in CI appears to be a viable idea.

  2. kallewoof commented at 11:28 PM on January 25, 2017: member

    Nice! Concept ACK.

  3. laanwj commented at 9:01 AM on January 26, 2017: member

    Concept ACK.

    Because these are meta-tools not part of Bitcoin Core's release cycle (and feature freezes, release branches, etc) I'd prefer to maintain these kinds of tools in an external repository, e.g. https://github.com/laanwj/bitcoin-maintainer-tools where the build-for-compare.py tool is.

  4. laanwj added the label Scripts and tools on Jan 26, 2017
  5. isle2983 commented at 5:05 PM on January 26, 2017: contributor

    @laanwj Fair enough. Though, I am trying to figure out the shape of a system to perform a suite of checks for every PR as part of the normal TravisCI run. I believe the script needs to live in the same repo for that to work.

    Would it make sense to have a suite of scripts under qa/? Perhaps presented by a script named qa/pull-tester/static-checks.py similar to rpc-tests.py? I see a similar pattern where there is one threshold of checks that must be passed, but then another category of checks that are probably more tentative (for whatever reason - false-positives, etc.).

  6. MarcoFalke commented at 9:11 PM on January 26, 2017: member

    I believe the script needs to live in the same repo for that to work.

    You can fetch any repo, just like the repo that is tested needs to be fetched as initial step.

    On a general note, I don't know if the following is possible, but I think it could serve as a reminder for the patch author to consider investigating if there are issue. At the same time it would not influence the result of the travis run:

    screenshot from 2017-01-26 21-59-33 <sub>(screenshot may not reflect reality)

  7. kallewoof commented at 12:01 AM on January 27, 2017: member

    Considering clang has a good possibility for throwing false positives, does it really belong in Travis? I can see it being used by devs periodically as a report tool but I don't think I'd want something to start throwing errors in builds that aren't errors.

  8. MarcoFalke commented at 12:08 AM on January 27, 2017: member

    does it really belong in Travis?

    No, for the reason you mentioned.

    On Fri, Jan 27, 2017 at 1:01 AM, kallewoof notifications@github.com wrote:

    Considering clang has a good possibility for throwing false positives, does it really belong in Travis? I can see it being used by devs periodically as a report tool but I don't think I'd want something to start throwing errors in builds that aren't errors.

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9632#issuecomment-275552577, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv7wFXfFjWM41lmwDypYkauTnDELRks5rWTPngaJpZM4LuC0E .

  9. isle2983 commented at 1:09 AM on January 27, 2017: contributor

    I have read the IRC logs from earlier today, so I am in touch with the range of concern in this area. I also agree with @laanwj sentiment that circling through the same arguments is a waste of rare talent needed for important problems. That is why I am noobing it up writing code and trying to stay out of people's way. I hope to demonstrate a working proposal and the PR can either be rejected outright or fixed/improved by incorporating whatever feedback comes (and the feedback so far has been fantastic).

    That being said, @MarcoFalke's mockup would be a great milestone. I see a trajectory of:

    1. getting a set of checks running in a consistent way (the TravisCI container looks perfect for that)
    2. establishing viewable metrics so they can be monitored

    After that, we can optionally continue into discussions of:

    1. improving the metrics with some bulk formatting in a minimally-disruptive way
    2. quantifying the amount of 'drift' in the wrong direction as PRs are merged over time
    3. enforcing some threshold in CI if it appears useful after seeing the actual data.

    For now, I plan on writing a similar script for clang-format to report on the state of the code w.r.t. src/.clang-format and also investigating what is possible/reasonable to do with .travis.yml along this trajectory.

  10. jtimon commented at 12:13 AM on February 1, 2017: contributor

    Concept ACK. Needs rebase. No strong opinion on putting it in contrib or https://github.com/laanwj/bitcoin-maintainer-tools. Perhaps that repo can be moved to https://github.com/bitcoin for more visibility.

  11. Implement clang_static_analysis.py
    A script for running 'scan-build' and exposing issues via static analysis.
    5c18bf2477
  12. isle2983 force-pushed on Feb 1, 2017
  13. MarcoFalke commented at 12:16 AM on February 1, 2017: member
  14. isle2983 commented at 12:20 AM on February 1, 2017: contributor

    Rebased. The conflict was with adjacent lines being removed in README.md by #9649.

    Also, this topic on automating analysis and checks continues in #9658.

  15. laanwj commented at 9:00 AM on February 2, 2017: member

    Same comments as here: #9658 (comment)

    Please move it to https://github.com/bitcoin-core/bitcoin-maintainer-tools

    Once this script becomes more mature and popular it can always be moved around, but we'll not merge it initially to qa/devtools.

  16. laanwj closed this on Feb 2, 2017

  17. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-15 15:15 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me