Skip to content

std.sort.pdq use correct recursion depth #24403

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

Closed
wants to merge 9 commits into from
Closed

Conversation

oittaa
Copy link
Contributor

@oittaa oittaa commented Jul 11, 2025

Fixes #24399

@mlugg
Copy link
Member

mlugg commented Jul 12, 2025

Take a look at the start of the stack trace:

thread 3949973 panic: reached unreachable code
/home/ci/actions-runner9/_work/zig/zig/lib/std/debug.zig:559:14: 0x55dc66b in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/ci/actions-runner9/_work/zig/zig/lib/std/math.zig:1264:11: 0x5644157 in log2_int__anon_26847 (zig)
    assert(x != 0);
          ^
/home/ci/actions-runner9/_work/zig/zig/lib/std/sort/pdq.zig:47:36: 0x6d8694f in pdqContext__anon_612807 (zig)
    const max_limit = math.log2_int(usize, b - a) + 1;

We're failing an assert(x != 0) in the call to log2_int. It's easy to guess that x will be the argument, but of course, you can always just check that in the code if you want:

zig/lib/std/math.zig

Lines 1261 to 1264 in 1c73b25

pub fn log2_int(comptime T: type, x: T) Log2Int(T) {
if (@typeInfo(T) != .int or @typeInfo(T).int.signedness != .unsigned)
@compileError("log2_int requires an unsigned integer, found " ++ @typeName(T));
assert(x != 0);

Yep -- so, we're calling log2_int with an argument of 0, which isn't allowed. Well, b - a == 0 is the same as b == a! So, pdqContext is being called with b == a; in other words, it's being asked to sort zero elements, which causes this problem becuase log2(0) isn't allowed.

Since there are no steps involved in sorting zero elements, you should be able to solve this simply by short-circuiting that case at the very top of the function like this:

if (a == b) return;

@alichraghi
Copy link
Contributor

I think it's better to assert len > 0in all sorting functions and check the length at call site

@mlugg
Copy link
Member

mlugg commented Jul 12, 2025

I don't see any reason sorting functions should require a non-empty input. Sorting a zero-length slice is well-defined with obvious behavior, and it requires either no special handling or in the worst case one trivial check.

@alichraghi
Copy link
Contributor

I suggest to check what the go implementation does

@andrewrk
Copy link
Member

Sorry, something as core as sorting needs a more disciplined approach. You can't just haphazardly push stuff to the CI and see what changes. Please investigate thoroughly before presenting your results by making a PR.

@andrewrk andrewrk closed this Jul 21, 2025
@oittaa oittaa deleted the patch-1 branch July 21, 2025 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std.sort.pdq is there a wrong limit for the recursion depth?
4 participants