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

Making (FixedBuffer)Allocator available at comptime #14931

Open
rsepassi opened this issue Mar 16, 2023 · 13 comments · Fixed by #14975
Open

Making (FixedBuffer)Allocator available at comptime #14931

rsepassi opened this issue Mar 16, 2023 · 13 comments · Fixed by #14975
Labels
standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@rsepassi
Copy link
Contributor

Zig Version

0.11.0-dev.1975+e17998b39

Steps to Reproduce and Observed Behavior

test "comptime alloc" {
  comptime {
    var buf: [1]u8 = undefined;
    var fba = std.heap.FixedBufferAllocator.init(&buf);
    var allocator = fba.allocator();
    var x = allocator.create(i8) catch unreachable;
    _ = x;
  }
}
.../std/mem/Allocator.zig:105:65: error: unable to evaluate comptime expression
    const slice = try self.allocAdvancedWithRetAddr(T, null, 1, @returnAddress());
                                                                ^~~~~~~~~~~~~~~~
...:11:29: note: called from here
    var x = allocator.create(i8) catch unreachable;
            ~~~~~~~~~~~~~~~~^~~~

Expected Behavior

Expected FixedBufferAllocator to work at comptime, which was suggested in this old issue #5873 (comment).

Goal is to be able to call code at comptime that expects an Allocator. Seemed sensible to use a FBA, but breaks because of the use of @returnAddress(). The impl of FBA does not use the return address passed down from Allocator (code).

@rsepassi rsepassi added the bug Observed behavior contradicts documented or intended behavior label Mar 16, 2023
@matu3ba
Copy link
Contributor

matu3ba commented Mar 16, 2023

Note, that as of now also static memory is not usable in all cases due to offset problems from comptime computed offsets. See #10920.

@InKryption
Copy link
Contributor

#13272 did set a precedent for this kind of explicit comptime friendliness, so I think this would be reasonable to look into, if the original proposal of the now-reopened #5873 is ultimately rejected.

mlugg added a commit to mlugg/zig that referenced this issue Mar 17, 2023
mlugg added a commit to mlugg/zig that referenced this issue Mar 17, 2023
andrewrk pushed a commit that referenced this issue Mar 17, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Mar 17, 2023
@rsepassi
Copy link
Contributor Author

rsepassi commented Mar 17, 2023

Unfortunately that didn't seem to be enough. After the returnAddress fix, there's a new error:

.../zig/lib/std/mem/Allocator.zig:218:5: error: TODO: Sema.zirMemset at comptime
    @memset(byte_ptr, undefined, byte_count);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../zig/lib/std/mem/Allocator.zig:105:52: note: called from here
    const slice = try self.allocAdvancedWithRetAddr(T, null, 1, @returnAddress());
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...:7:29: note: called from here
    var x = allocator.create(i8) catch unreachable;

zirMemset unimplemented at comptime.

@rsepassi
Copy link
Contributor Author

cc @mlugg

@mlugg
Copy link
Member

mlugg commented Mar 18, 2023

Oh that's silly. I can fix that

@andrewmilson
Copy link

I'm also having issues with FixedBufferAllocator at comptime (zig 0.11.0-dev.2165+8f481dfc3). There's an additional issue when the types don't share the same alignment as u8.

to reproduce:

test "comptime alloc" {
    const std = @import("std");
    comptime {
        var buf: [1000]u8 = undefined;
        var fba = std.heap.FixedBufferAllocator.init(&buf);
        var allocator = fba.allocator();
        var list = std.ArrayList(u64).init(allocator);
        list.append(1) catch unreachable;
    }
}
.../lib/zig/std/mem.zig:3279:18: error: unable to evaluate comptime expression
    const addr = @ptrToInt(ptr);
                 ^~~~~~~~~~~~~~
.../lib/zig/std/mem.zig:3279:28: note: operation is runtime due to this operand
    const addr = @ptrToInt(ptr);
                           ^~~
.../lib/zig/std/heap.zig:420:50: note: called from here
        const adjust_off = mem.alignPointerOffset(self.buffer.ptr + self.end_index, ptr_align) orelse return null;
                           ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
src/main.zig:98:20: note: called from here
        list.append(1) catch unreachable;

@andrewmilson
Copy link

cc @mlugg

@mlugg
Copy link
Member

mlugg commented Mar 19, 2023

After looking at this last night, I realised there are some more fundamental issues with the comptime memory model which currently block comptime allocators from working properly. @rsepassi, would you be able to re-open this issue?

@rsepassi
Copy link
Contributor Author

Thanks for digging into it. I don't seem to be able to reopen the issue. Has to be a maintainer?

@andrewrk andrewrk reopened this Mar 19, 2023
truemedian pushed a commit to truemedian/zig that referenced this issue Mar 30, 2023
@rsepassi
Copy link
Contributor Author

rsepassi commented Apr 3, 2023

To work around this for my use case, I made a comptime bump allocator and ArrayList that aren't quite drop-in replacements but were close enough for my own code. I also found that I needed to use packed structs with this allocator to ensure layout was known at comptime.

https://gist.github.com/rsepassi/d356ea5cfebf37bd9ba8c5d175a7ea30

@rsepassi
Copy link
Contributor Author

rsepassi commented Apr 3, 2023

Note the need for packed structs was because of:

ziglib/libcomptime/comptimearray.zig:85:28: error: comptime mutation of a reinterpreted pointer requires type 'view_ast.Ast.NameLookupSingle' to have a well-defined memory layout
            new_item_ptr.* = item;

Sounds like this is related to the comptime memory model limitations that @mlugg was referring to.

There's an old open issue re the comptime memory model that seems related, though I'm unsure: #9646.

@mlugg
Copy link
Member

mlugg commented Apr 3, 2023

Sounds like this is related to the comptime memory model limitations that @mlugg was referring to.

Yup, exactly correct. Comptime tries to stop you from relying on explicitly undefined memory layouts (e.g. the layouts of auto-layout structs, optionals, error unions), but the way it does this is too heavy-handed and catches allocators out, since they try to reinterpret a byte buffer as a pointer to another type, and if that one has undefined layout the cast is currently just considered illegal.

There's an old open issue re the comptime memory model that seems related, though I'm unsure: #9646.

That is indeed related! The rules explained in that issue turn out to be a little too restrictive, so to make this work we need to deviate from them a bit. I do have a plan to achieve this, which Andrew agreed made sense, but it's kinda complex and might be a while before I can get around to implementing it.

@rsepassi
Copy link
Contributor Author

rsepassi commented Apr 4, 2023

Got it, thank you. I'd be happy for now if I could get a not-really-replacement Allocator and ArrayList working (similar to https://gist.github.com/rsepassi/d356ea5cfebf37bd9ba8c5d175a7ea30), but I seem to have hit some bug and I can't quite tell if it's fundamentally tied up with the memory model restrictions or if it's a simple fix: #15165.

@andrewrk andrewrk modified the milestones: 0.11.0, 0.12.0 Jul 20, 2023
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. and removed bug Observed behavior contradicts documented or intended behavior labels Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants