New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevector: fix 2 bugs in currently unreached code paths #7888

Merged
merged 3 commits into from Apr 18, 2016

Conversation

Projects
None yet
3 participants
@kazcw
Contributor

kazcw commented Apr 16, 2016

It looks like maybe this swap method is from an earlier iteration of prevector that used the LSB of size as a "direct/indirect" tag. The bad path isn't ever hit because in all current instances of swapping two prevectors, one is newly value-initialized and thus has an even size (0).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 16, 2016

Member

Concept ACK.
I think we need a unit test that fail before this, and succeed after this.

Member

laanwj commented Apr 16, 2016

Concept ACK.
I think we need a unit test that fail before this, and succeed after this.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 16, 2016

Member

Nice catch, thanks.

Code review ACK, but agree that a test for this behaviour would be welcome.

Member

sipa commented Apr 16, 2016

Nice catch, thanks.

Code review ACK, but agree that a test for this behaviour would be welcome.

kazcw added some commits Apr 13, 2016

prevector: destroy elements only via erase()
Fixes a bug in which pop_back did not call the deleted item's destructor.

Using the most general erase() implementation to implement all the others
prevents similar bugs because the coupling between deallocation and destructor
invocation only needs to be maintained in one place.
Also reduces duplication of complex memmove logic.
prevector::swap: fix (unreached) data corruption
swap was using an incorrect condition to determine when to apply an optimization
(not swapping the full direct[] when swapping two indirect prevectors).

Rather than correct the optimization I'm removing it for simplicity. Removing
this optimization minutely improves performance in the typical (currently only)
usage of member swap(), which is swapping with a freshly value-initialized
object.
test prevector::swap
- add a swap operation to prevector tests (fails due to broken prevector::swap)
- fix 2 prevector test operation conditions that were impossible
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 18, 2016

Member

Thanks for adding the test. I verified that it passes with, and fails without a7af72a.
tACK a7af72a

Member

laanwj commented Apr 18, 2016

Thanks for adding the test. I verified that it passes with, and fails without a7af72a.
tACK a7af72a

@laanwj laanwj merged commit a7af72a into bitcoin:master Apr 18, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Apr 18, 2016

Merge #7888: prevector: fix 2 bugs in currently unreached code paths
a7af72a prevector::swap: fix (unreached) data corruption (Kaz Wesley)
4ed41a2 test prevector::swap (Kaz Wesley)
1e2c29f prevector: destroy elements only via erase() (Kaz Wesley)

@kazcw kazcw deleted the kazcw:pvfix branch Apr 22, 2016

sickpig referenced this pull request in sickpig/BitcoinUnlimited Mar 9, 2018

Port Core #7888: prevector: fix 2 bugs in currently unreached code paths
a7af72a prevector::swap: fix (unreached) data corruption (Kaz Wesley)
4ed41a2 test prevector::swap (Kaz Wesley)
1e2c29f prevector: destroy elements only via erase() (Kaz Wesley)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment