allocator ownSlice fails - workaround adding print to test!!!

[dvmason]

Zig Version

0.10.0-dev.1091+7fc8dd66

Steps to Reproduce

Pull Zag-Research/Zag-Smalltalk@00bddf0 and comment/uncomment the print at the end of heap.zig. Then zig test heap.zig
With the print in, everything is cool.
With the print commented, the allocator fails a sanity check for "ownsSlice".

This is on:
Darwin MacBook-Pro-2.local 18.7.0 Darwin Kernel Version 18.7.0: Tue Jun 22 19:37:08 PDT 2021; root:xnu-4903.278.70~1/RELEASE_X86_64 x86_64

Expected Behavior

Pretty sure there's no error in that part of the code.

Very weird that a print in a test can make the allocator happy!

Actual Behavior

3/6 test "three object allocator"... thread 1798710 panic: reached unreachable code
/usr/local/lib/zig/std/debug.zig:234:14: 0x10b469698 in std.debug.assert (test)
    if (!ok) unreachable; // assertion failure
             ^
/usr/local/lib/zig/std/heap.zig:822:15: 0x10b47940d in std.heap.FixedBufferAllocator.free (test)
        assert(self.ownsSlice(buf)); // sanity check
              ^
/usr/local/lib/zig/std/mem/Allocator.zig:94:13: 0x10b46ca9a in std.mem.Allocator.gen.freeImpl (test)
            @call(.{ .modifier = .always_inline }, freeFn, .{ self, buf, buf_align, ret_addr });
            ^
/usr/local/lib/zig/std/mem/Allocator.zig:177:28: 0x10b4796c6 in std.mem.Allocator.free (test)
    return self.vtable.free(self.ptr, buf, buf_align, ret_addr);
                           ^
/Users/dmason/git/AST-Smalltalk/zig/heap.zig:193:28: 0x10b479561 in Arena.deinit (test)
        self.allocator.free(self.allocated);
                           ^
/Users/dmason/git/AST-Smalltalk/zig/heap.zig:272:26: 0x10b474376 in TestArena.deinit (test)
        self.arena.deinit();
                         ^
/Users/dmason/git/AST-Smalltalk/zig/heap.zig:298:20: 0x10b46d23c in TestArena.finish (test)
        self.deinit();
                   ^
/Users/dmason/git/AST-Smalltalk/zig/heap.zig:318:27: 0x10b46bd99 in test "three object allocator" (test)
    defer testArena.finish();
                          ^
/usr/local/lib/zig/std/special/test_runner.zig:80:28: 0x10b46d760 in std.special.main (test)
        } else test_fn.func();
                           ^
error: the following test command crashed:
zig-cache/o/58ff5bc14db16ae03eaf3ee16d8d6d36/test /usr/local/bin/zig

[Vexu]

On line 264 you're taking the address of a local variable and returning it. Adding the prints messes with the stack just enough to make it work properly.

[dvmason]

Vexu refers to this:

        const output = try testing.allocator.alloc(Object,expected.len);
        const allocator = @import("std").heap.FixedBufferAllocator.init(mem.sliceAsBytes(output)).allocator();

where I thought I was passing the memory provided by the testing allocator to the fixed buffer allocator.

[Vexu]

where I thought I was passing the memory provided by the testing allocator to the fixed buffer allocator.

You are doing that but you're also creating a temporary FixedBufferAllocator instance on the stack, taking a pointer to it via the method call syntax (.allocator()) and then returning that pointer as a field in Arena.
To make this work you need to store the FixedBufferAllocator in the Arena instead of just storing the Allocator interface.

[dvmason]

So save @import("std").heap.FixedBufferAllocator.init(mem.sliceAsBytes(output)) rather than @import("std").heap.FixedBufferAllocator.init(mem.sliceAsBytes(output)).allocator() ?

[dvmason]

I need to save something like the allocator somewhere so I can free the value later.

[Vexu]

Yup, this used to cause #591 but then the allocator interface was changed and now it's less obvious that you're taking a pointer to a temporary stack variable.

[dvmason]

OK, I found it in the code in zig/std/heap.zig for FixedBufferAllocator - it passes a * FixedBufferAllocator to Allocator.init.

I can't make the code quite as generic as I wanted, but maybe I'm seeing that I don't need to....

Subtle! Thanks for the help understanding this.

[SpexGuy]

FWIW this will become a compile error once stack temporaries are changed to be implicitly const.