Page MenuHomePhabricator

[SECP256K1] Fix -Wmaybe-uninitialized in tests
AbandonedPublic

Authored by Fabien on May 29 2020, 15:09.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

The test_ecmult_multi_batch_single is relying on the empty scratch
memory to cause the ecmult algorithm exit early and fail. The tests let
the scalar and point to multiply uninitialized, which is fine since they
will not be used by the algorithm. Unfortunately GCC fails to detect
this situation and will raise -Wmaybe-uninitialized false positive
warnings. Interestingly not all versions are affected, but there is a
handful of bugs with this warning on GCC:
https://gcc.gnu.org/bugzilla/buglist.cgi?quicksearch=maybe-uninitialized

This diffs intializes the operands with random values. It also reduces
the variables sizes because the multiplication applies on a single
point.

Test Plan

With GCC as a compiler (tested with GCC 10.1):

ninja check-secp256k1

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_warning_maybe_uninitialized_secp256k1_tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 11082
Build 20600: Default Diff Build & Tests
Build 20599: Build without Wallet
Build 20598: Build with clang-10
Build 20597: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.May 29 2020, 15:09
deadalnix requested changes to this revision.May 29 2020, 21:25
deadalnix added a subscriber: deadalnix.

I would rather avoid taking ownership of this. The part of libsecp256k1 we have ownership over are very well delimited and separated from the rest of the codebase. IMO this is a feature.

I would suggest you go about this by either:

  • Disable this warning for libsecp256k1 and report the problem upstream.
  • Submit a patch upstream.
This revision now requires changes to proceed.May 29 2020, 21:25

I would rather avoid taking ownership of this. The part of libsecp256k1 we have ownership over are very well delimited and separated from the rest of the codebase. IMO this is a feature.

I would suggest you go about this by either:

  • Disable this warning for libsecp256k1 and report the problem upstream.
  • Submit a patch upstream.

+1 to submitting a patch upstream. We should build this up in our culture to do this more often. The cost is a little higher, but it provides impact outside of just our team and gives us some visibility.

It appears that core doesn't have this warning due to D6309.