Page MenuHomePhabricator

[secp256k1] scratch: add stack frame support
ClosedPublic

Authored by deadalnix on Jan 14 2020, 01:20.

Details

Summary

This is a backport of libsecp256k1's PR523

Test Plan
ninja check-secp256k1

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Jan 15 2020, 11:21
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

Should be <= since scratch->frame is a counter and not an index ?

src/secp256k1/src/tests.c
275 ↗(On Diff #15425)

secp256k1_scratch_allocate_frame doesn't take a boolean argument, this looks like a typo for CHECK(secp256k1_scratch_allocate_frame(scratch, 500, 1) == 1);

This revision now requires changes to proceed.Jan 15 2020, 11:21
src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

This is as per the source material. This is correct if the frames are counted from 0; which they most likely are.

src/secp256k1/src/tests.c
275 ↗(On Diff #15425)

Even though this isn't what's in the PR, this seems to be the intended test.

Update erroneous test case

src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

This would be inconsistent with, for example, then assertion in secp256k1_scratch_destroy above: VERIFY_CHECK(scratch->frame == 0);. If the frames are counted from 0, then there might still be an allocated frame when the scratch is destroyed, causing a memory leak.

src/secp256k1/src/scratch_impl.h
48 ↗(On Diff #15425)

You are correct. This is counting, but the code is correct anyways, because if we are going to allocate a new frame, then we want the frame count to be bellow the max, that that at the end of the process we reach at most the max number of frames.

This revision is now accepted and ready to land.Jan 15 2020, 13:51