Avoid static analyzer warnings regarding uninitialized arguments #10735

Merged
merged 1 commit into from Jul 16, 2017

Conversation

Projects
None yet
6 participants
Contributor

practicalswift commented Jul 3, 2017 edited

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
$
src/bench/base58.cpp
- 17, 79, 8, 99, 150, 189, 208, 162, 22, 23, 203, 163, 36, 58, 147,
- 227, 139, 2, 215, 100, 91, 38, 11, 141, 253, 40, 117, 21, 16, 90,
- 200, 24
+ std::array<unsigned char, 32> buff = {
@sipa

sipa Jul 3, 2017

Owner

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

@practicalswift

practicalswift Jul 3, 2017

Contributor

@sipa Good point! Fixed! :-)

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

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

@sipa

sipa Jul 14, 2017

Owner

std::array stores on the stack

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

static implies not-on-stack, no?

@sipa

sipa Jul 15, 2017

Owner

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

@TheBlueMatt

TheBlueMatt Jul 15, 2017

Contributor

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.

fanquake added the Refactoring label Jul 5, 2017

Member

jonasschnelli commented Jul 13, 2017

utACK dcc0e0f

Contributor

paveljanik commented Jul 13, 2017

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.
@practicalswift practicalswift Avoid static analyzer warnings regarding uninitialized arguments
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
$
```
6835cb0
Contributor

practicalswift commented Jul 15, 2017

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

Contributor

paveljanik commented Jul 15, 2017

utACK 6835cb0

Owner

sipa commented Jul 15, 2017

utACK 6835cb0

Contributor

TheBlueMatt commented Jul 16, 2017

utACK 6835cb0

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sipa sipa added a commit that referenced this pull request Jul 16, 2017

@sipa sipa Merge #10735: Avoid static analyzer warnings regarding uninitialized …
…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
5654946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment