Crashes/bugs due to parameterised tests receiving incorrect context

[Kylar]

Zig Version

0.14.0

Steps to Reproduce and Observed Behavior

Using branch zig-0.14.0, commit fb3ecae be4ca3f

Code and sample output below.

Code to reproduce:

const BenchmarkDebug = struct {
    num: usize,

    pub fn run(self: @This(), allocator: Allocator) void {
        std.debug.print("oopsies. {d}\n", .{ self.num });
        _ = allocator;
    }
};

test "benchmarkdebug" {
    var runner = zbench.Benchmark.init(std.testing.allocator, .{});
    defer runner.deinit();

    const test_case = BenchmarkDebug{ .num = 1025 };
    try runner.addParam("Benchmark1 ", &test_case, .{});

    try runner.run(std.io.getStdErr().writer());

    std.debug.print("address: {*}\n", .{ &test_case });
    std.debug.print("address: {d}\n", .{ @intFromPtr(&test_case) });
}

Sample output:

...
oopsies. 19043096
oopsies. 19043096
Benchmark1 100000 284.664ms 2.846us ± 3.02us (2us ... 301.5us) 2.7us 15.3us 16.8us
address: main.BenchmarkDebug@1229318
address: 19043096

Expected Behavior

I expect the run function in my repro code to print the value in my context (1025), not its address (19043096).

I believe the run function called by parameterised tests should be typed fn (*T, Allocator) void or similar, but it is currently forced to have the type fn (T, Allocator) void.

[bens]

That's very odd, I just tried this with zig 0.14.0 and got the expected result. I dropped your benchmark into the parameterised benchmark example file, this is the whole file:

const std = @import("std");
const zbench = @import("zbench");

const BenchmarkDebug = struct {
    num: usize,

    pub fn run(self: @This(), allocator: std.mem.Allocator) void {
        std.debug.print("oopsies. {d}\n", .{self.num});
        _ = allocator;
    }
};

pub fn main() !void {
    const stdout = std.io.getStdOut().writer();
    var bench = zbench.Benchmark.init(std.heap.page_allocator, .{});
    defer bench.deinit();

    const test_case = BenchmarkDebug{ .num = 1025 };
    try bench.addParam("BenchmarkDebug", &test_case, .{});

    try stdout.writeAll("\n");
    try bench.run(stdout);
}

Output is

benchmark              runs     total time     time/run (avg ± σ)     (min ... max)                p75        p99        p995      
-----------------------------------------------------------------------------------------------------------------------------
oopsies. 1025
oopsies. 1025
[...]
oopsies. 1025
oopsies. 1025
BenchmarkDebug         37566    2.573s         68.495us ± 1.682ms     (4.261us ... 256.238ms)      16.343us   23.397us   45.328us  
address: parameterised.BenchmarkDebug@102b348
address: 16954184

I notice you're running your benchmarks in a test block, you are avoiding stdout by using std.debug.print but I wonder if there are other issues with running benchmarks in a test, since that's not so well tested with its stdout problems.

Not making the self parameter a pointer means that the context can't be mutated from within the benchmark, which I thought of as a feature. It shouldn't produce this kind of problem though.

[bens]

I put this at the end of zbench.zig and ran zig build test with zig 0.14.0 and still got the same result. So I'm really not sure what's going on there.

const BenchmarkDebug = struct {
    num: usize,

    pub fn run(self: @This(), allocator: std.mem.Allocator) void {
        std.debug.print("oopsies. {d}\n", .{self.num});
        _ = allocator;
    }
};

test "benchmarkdebug" {
    var runner = Benchmark.init(std.testing.allocator, .{});
    defer runner.deinit();

    const test_case = BenchmarkDebug{ .num = 1025 };
    try runner.addParam("Benchmark1 ", &test_case, .{});

    try runner.run(std.io.getStdErr().writer());

    std.debug.print("address: {*}\n", .{&test_case});
    std.debug.print("address: {d}\n", .{@intFromPtr(&test_case)});
}

[...]
oopsies. 1025
oopsies. 1025
Benchmark1             100000   505.725ms      5.057us ± 2.798us      (1.886us ... 436.86us)       5.308us    10.895us   11.035us
address: zbench.BenchmarkDebug@102f3a8
address: 16970664

[bens]

I tried using the patch you mention, it's from November last year and doesn't compile at all with 0.14.0. I'd suggest checking whether it's an issue for you against an up to date version of the library first. The version in main worked just fine for me apart from a small update to build.zig.zon.

[Kylar]

Thanks for the replies. I think I have figured it out.

My build.zig had .addExecutable(.{ ..., .use_llvm = false }). With false the bug appears, with true it doesn't.

I tried modifying zbench to use the code below, and changed run to receive a pointer, and can confirm that works with and without LLVM. And by using *const you still get your desired (shallow) immutability. 😃

        // Check the benchmark parameter has a well typed run function.
-        _ = @as(fn (T, std.mem.Allocator) void, T.run);
+        _ = @as(fn (*const T, std.mem.Allocator) void, T.run);

Ultimately to me it makes sense to be a pointer, because at the call site zbench uses the type ParameterisedFunc = *const fn (*const anyopaque, std.mem.Allocator) void which has a pointer parameter, and passing a pointer to a function not expecting a pointer really should not work... they should both be pointers, or should both not be pointers.

Also my apologies, I was actually using the latest commit (be4ca3f) in branch zig-0.14.0 but accidentally listed the wrong one in the opening message. I've now tested main as well and it behaves the same.

[FObersteiner]

Ultimately to me it makes sense to be a pointer, because at the call site zbench uses the type ParameterisedFunc = *const fn (*const anyopaque, std.mem.Allocator) void which has a pointer parameter, and passing a pointer to a function not expecting a pointer really should not work... they should both be pointers, or should both not be pointers.

That sounds reasonable to me. My vote would be to use a *const, although it seems uncommon in Zig codebases that an allocator is passed by const pointer. However IIRC the compiler can still decide to pass-by-pointer even if the code specifies pass-by-value, plus allocators are just interfaces that point to the concrete implementation anyways. Related post on ziggit.dev.

[Kylar]

That sounds reasonable to me. My vote would be to use a *const, although it seems uncommon in Zig codebases that an allocator is passed by const pointer.

Just to be clear it's not the allocator param that needs to change, it's the context param.

However IIRC the compiler can still decide to pass-by-pointer even if the code specifies pass-by-value...

Now that you mention it, that would explain the difference in behaviour if LLVM build is changing run to pass-by-pointer while the non-LLVM build keeps pass-by-value.

[FObersteiner]

right, I should be reading properly first. Anyways, doesn't change my "vote" though; because of this "pass-by-value could actually be pass-by-pointer" thing I tend to use *const whenever possible for non-primitive types.

[bens]

Yeah, *const would be fine too, and it does make more sense really, it's doing a @ptrCast in runImpl as well which all makes sense.