Skip to content
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

Bug fix & run overflow correction much more frequently in tests #2603

Merged
merged 1 commit into from
May 4, 2021

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented May 3, 2021

  • Fix overflow correction when windowLog < cycleLog. Previously, we
    got the correction wrong in this case, and our chain tables and binary
    trees would be corrupted. Now, we work as long as maxDist is a power
    of two, by adding MAX(maxDist, cycleSize) to our indices.
  • When ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY is defined to non-zero
    run overflow correction as frequently as allowed without impacting
    compression ratio.
  • Enable ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY in fuzzer and
    zstreamtest as well as all the OSS-Fuzz fuzzers. This has a 5-10%
    speed penalty at most, which seems reasonable.

This change allows both the OSS-Fuzz fuzzers and fuzzer and zstreamtest to all catch the ZSTD_reduceIndex() bug, as well as the assertion failure.

@terrelln
Copy link
Contributor Author

terrelln commented May 3, 2021

Tests failed because it caught the ZSTD_reduceIndex() bug, since I hadn't yet rebased onto dev.

* Fix overflow correction when `windowLog < cycleLog`. Previously, we
  got the correction wrong in this case, and our chain tables and binary
  trees would be corrupted. Now, we work as long as `maxDist` is a power
  of two, by adding `MAX(maxDist, cycleSize)` to our indices.
* When `ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY` is defined to non-zero
  run overflow correction as frequently as allowed without impacting
  compression ratio.
* Enable `ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY` in `fuzzer` and
  `zstreamtest` as well as all the OSS-Fuzz fuzzers. This has a 5-10%
  speed penalty at most, which seems reasonable.
terrelln added a commit to terrelln/zstd that referenced this pull request May 3, 2021
Switch from `-T0` to the default `-T1` which significantly reduces
memory usage for level 19 when there are many cores. This fixes
32-bit issues of running out of address space.

Fixes facebook#2603.
* code much more frequently. This is very inefficient, and should only be
* used for tests and fuzzers.
*/
#ifndef ZSTD_WINDOW_OVERFLOW_CORRECT_FREQUENTLY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this macro be documented ?
I presume it's only meant to be used by maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it really deserves documentation in any README.md, since it shouldn't be enabled in anything but tests. It is automatically enabled for fuzzers, which have a standard macro FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.

If we wanted this to be used more widely, I think we would want to have a general debug flag like DEBUGLOG, except for enabling "expensive" checks.

Copy link
Contributor

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good design

@terrelln terrelln merged commit 0b370e9 into facebook:dev May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants