Avoid static analyzer warnings regarding uninitialized arguments by practicalswift · Pull Request #10735 · bitcoin/bitcoin

practicalswift · 2017-07-03T15:50:55Z

Avoid static analyzer warnings regarding "Function call argument is a pointer to uninitialized value" in cases where we are intentionally using such arguments.

This is achieved by using f(b.begin(), b.end()) (std::array<char, N>) instead of f(b, b + N) (char b[N]).

Rationale:

Reduce false positives by guiding static analyzers regarding our intentions.

Before this commit:

$ clang-tidy-3.5 -checks=* src/bench/base58.cpp
bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage]
        EncodeBase58(b, b + 32);
        ^
$ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp
bench/verify_script.cpp:59:5: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage]
    key.Set(vchKey, vchKey + 32, false);
    ^
$

After this commit:

$ clang-tidy-3.5 -checks=* src/bench/base58.cpp
$ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp
$

sipa · 2017-07-03T15:56:12Z

src/bench/base58.cpp

While you're at it, make these static const ?

@sipa Good point! Fixed! :-)

tiny nit: I might prefer this if it kept the benchmarked-against data on stack instead of heap, but it doesnt matter much.

std::array stores on the stack

static implies not-on-stack, no?

@TheBlueMatt static const is just statically allocated by the binary, not even on the heap.

Yes, indeed, my point was to prefer stack over binary or other allocations. It shouldnt matter cause the memory usage of these benchmarks should be trivial, so whatever, it doesnt matter.

jonasschnelli · 2017-07-13T07:27:48Z

utACK dcc0e0f2c93888f904001703a7ef6be11f61a9e4

paveljanik · 2017-07-13T08:36:57Z

OS X, Apple LLVM version 7.0.2:

  CXX      bench/bench_bench_bitcoin-verify_script.o
bench/verify_script.cpp:58:48: error: implicit instantiation of undefined template 'std::__1::array<unsigned char, 32>'
    static const std::array<unsigned char, 32> vchKey = {
                                               ^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__tuple:95:65: note: template is declared here
template <class _Tp, size_t _Size> struct _LIBCPP_TYPE_VIS_ONLY array;
                                                                ^
1 error generated.

Avoid static analyzer warnings regarding "Function call argument is a pointer to uninitialized value" in cases where we are intentionally using such arguments. This is achieved by using ... `f(b.begin(), b.end())` (`std::array<char, N>`) ... instead of ... `f(b, b + N)` (`char b[N]`) Rationale: * Reduce false positives by guiding static analyzers regarding our intentions. Before this commit: ``` $ clang-tidy-3.5 -checks=* src/bench/base58.cpp bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] EncodeBase58(b, b + 32); ^ $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp bench/verify_script.cpp:59:5: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] key.Set(vchKey, vchKey + 32, false); ^ $ ``` After this commit: ``` $ clang-tidy-3.5 -checks=* src/bench/base58.cpp $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp $ ```

practicalswift · 2017-07-15T12:27:34Z

@paveljanik Missing #include <array> added. Thanks for reviewing! :-)

paveljanik · 2017-07-15T13:05:36Z

utACK 6835cb0

sipa · 2017-07-15T18:02:56Z

utACK 6835cb0

TheBlueMatt · 2017-07-16T15:19:59Z

utACK 6835cb0

…arguments 6835cb0 Avoid static analyzer warnings regarding uninitialized arguments (practicalswift) Pull request description: Avoid static analyzer warnings regarding _"Function call argument is a pointer to uninitialized value"_ in cases where we are intentionally using such arguments. This is achieved by using `f(b.begin(), b.end())` (`std::array<char, N>`) instead of `f(b, b + N)` (`char b[N]`). Rationale: * Reduce false positives by guiding static analyzers regarding our intentions. Before this commit: ```shell $ clang-tidy-3.5 -checks=* src/bench/base58.cpp bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] EncodeBase58(b, b + 32); ^ $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp bench/verify_script.cpp:59:5: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] key.Set(vchKey, vchKey + 32, false); ^ $ ``` After this commit: ```shell $ clang-tidy-3.5 -checks=* src/bench/base58.cpp $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp $ ``` Tree-SHA512: 5814a320ca8b959d0954bb64393424bcad73f942d2e988de1cd6788f39153b93900325532f2e340de02d740a3953385d212ae08e7ec72bb4c394a40475f251df

…alized arguments 6835cb0 Avoid static analyzer warnings regarding uninitialized arguments (practicalswift) Pull request description: Avoid static analyzer warnings regarding _"Function call argument is a pointer to uninitialized value"_ in cases where we are intentionally using such arguments. This is achieved by using `f(b.begin(), b.end())` (`std::array<char, N>`) instead of `f(b, b + N)` (`char b[N]`). Rationale: * Reduce false positives by guiding static analyzers regarding our intentions. Before this commit: ```shell $ clang-tidy-3.5 -checks=* src/bench/base58.cpp bench/base58.cpp:23:9: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] EncodeBase58(b, b + 32); ^ $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp bench/verify_script.cpp:59:5: warning: Function call argument is a pointer to uninitialized value [clang-analyzer-core.CallAndMessage] key.Set(vchKey, vchKey + 32, false); ^ $ ``` After this commit: ```shell $ clang-tidy-3.5 -checks=* src/bench/base58.cpp $ clang-tidy-3.5 -checks=* src/bench/verify_script.cpp $ ``` Tree-SHA512: 5814a320ca8b959d0954bb64393424bcad73f942d2e988de1cd6788f39153b93900325532f2e340de02d740a3953385d212ae08e7ec72bb4c394a40475f251df

Micro-benchmarking framework part 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6733 - bitcoin/bitcoin#6770 - bitcoin/bitcoin#6892 - Excluding changes to `src/policy/policy.h` which we don't have yet. - bitcoin/bitcoin#7934 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#8039 - bitcoin/bitcoin#8107 - bitcoin/bitcoin#8115 - bitcoin/bitcoin#8914 - Required resolving several merge conflicts in code that had been refactored upstream. The changes were simple enough that I decided it was okay to impose merge conflicts on pulling in those refactors later. - bitcoin/bitcoin#9200 - bitcoin/bitcoin#9202 - Adds support for measuring CPU cycles, which is later removed in an upstream PR after the refactor. I am including it to reduce future merge conflicts. - bitcoin/bitcoin#9281 - Only changes to `src/bench/bench.cpp` - bitcoin/bitcoin#9498 - bitcoin/bitcoin#9712 - bitcoin/bitcoin#9547 - bitcoin/bitcoin#9505 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#9792 - Just the benchmark, not the performance improvements. - bitcoin/bitcoin#10272 - bitcoin/bitcoin#10395 - Only changes to `src/bench/` - bitcoin/bitcoin#10735 - Only changes to `src/bench/base58.cpp` - bitcoin/bitcoin#10963 - bitcoin/bitcoin#11303 - Only the benchmark backend change. - bitcoin/bitcoin#11562 - bitcoin/bitcoin#11646 - bitcoin/bitcoin#11654 This pulls in all changes to the micro-benchmark framework prior to December 2017, when it was rewritten. The rewrite depends on other upstream PRs we have not pulled in yet. This does not pull in all benchmarks prior to December 2017. It leaves out benchmarks that either test code we do not have yet (except for the `FastRandomContext` refactor, which I decided to pull in), or would require rewrites to work with our changes to the codebase.

sipa reviewed Jul 3, 2017

View reviewed changes

practicalswift force-pushed the std-array branch from 397af13 to dcc0e0f Compare July 3, 2017 20:00

fanquake added the Refactoring label Jul 5, 2017

practicalswift force-pushed the std-array branch from dcc0e0f to 6835cb0 Compare July 15, 2017 12:26

sipa merged commit 6835cb0 into bitcoin:master Jul 16, 2017

practicalswift mentioned this pull request Oct 4, 2017

Fix invalid checks (NULL checks after dereference, redundant checks, etc.) #10898

Merged

str4d mentioned this pull request Feb 22, 2019

Micro-benchmarking framework part 1 zcash/zcash#3858

Merged

sickpig mentioned this pull request Apr 28, 2020

Update benchmark suite BitcoinUnlimited/BitcoinUnlimited#2191

Merged

practicalswift deleted the std-array branch April 10, 2021 19:31

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

practicalswift commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa Jul 3, 2017

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 3, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Jul 15, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 15, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jul 13, 2017

Uh oh!

paveljanik commented Jul 13, 2017

Uh oh!

practicalswift commented Jul 15, 2017

Uh oh!

paveljanik commented Jul 15, 2017

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

TheBlueMatt commented Jul 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Jul 3, 2017 •

edited

Loading