access to Allocator inside wrapped Zig functions

[nurpax]

When calling a wrapped Zig function from within Lua, lua.allocator seems to be always null:

pub const Player = struct {
    score: i32,

    pub fn init() Player {
        return Player{
            .score = 0,
        };
    }

    pub fn incScore(self: @This()) void {
        self.score += 1;
    }
};

fn newPlayer(lua: *Lua) i32 {
    std.debug.print("newPlayer zig\n", .{});
    if (lua.allocator) |a| {
        const p = a.create(Player) catch unreachable;
        const udata = lua.newUserdata(*Player);
        udata.* = p;
    } else {
        std.debug.print("lua allocator is null?\n", .{});
        return 0;
    }
    return 1;
}

pub fn register(lua: *Lua) void {
    lua.pushFunction(ziglua.wrap(newPlayer), "newPlayer");
    lua.setGlobal("newPlayer");
}

I think it's due to how wrapZigFn creates a new Lua context:

/// Wrap a ZigFn in a CFn for passing to the API
fn wrapZigFn(comptime f: ZigFn) CFn {
    return struct {
        fn inner(state: ?*LuaState) callconv(.C) c_int {
            // this is called by Lua, state should never be null
            var lua: Lua = .{ .state = state.? };
            return @call(.always_inline, f, .{&lua});
        }
    }.inner;
}

I suppose it should somehow setup the allocator field too.

IMO having a working allocator within the wrapped functions would be useful, so that I can go ahead and use whatever allocator is passed through the lua state.

[natecraddock]

IMO having a working allocator within the wrapped functions would be useful, so that I can go ahead and use whatever allocator is passed through the lua state.

I agree. I have a few ideas on how to do this, though I'll prioritize some bigger changes first

[atweiden]

It’s possible to do, e.g.

https://git.sr.ht/~ioiojo/star/tree/master/item/src/utils/lua.zig#L504

https://git.sr.ht/~ioiojo/star/tree/master/item/src/utils/lua.zig#L2564

https://git.sr.ht/~ioiojo/star/tree/master/item/src/config/system.zig#L177

[natecraddock]

I've been looking at this the last few days, and I'm not sure the best way to implement it. There is a lot of ambiguity. I'll list my thoughts, and I would appreciate any feedback!

As @atweiden suggests, we could add a simple getAllocator() function to the Lua struct.

pub fn getAllocator(lua: *Lua) *std.mem.Allocator {
    var data: *anyopaque = undefined;
    _ = lua.getAllocFn(&data);
    return ziglua.opaqueCast(std.mem.Allocator, data);
}

This would be the simplest solution.

But I think it would be nice if lua.allocator was directly accessible without needing a function call. Setting this in all of the "wrap" functions is straightforward, but there are several edge cases that I don't know what to do about.

In addition to the standard Lua.init() function, there are also Lua.newState() and Lua.newStateLibc() functions.

If the Lua state is created with Lua.newState(), there is no guarantee that the data pointer is a Zig allocator. And with Lua.newStateLibc() it will be null.

And in all cases, the existence of Lua.setAllocF() means the data can be set to anything at any time. This would even cause issues with the lua.getAllocator() suggestion above.

So here are the ideas I have:

1. Add a lua.getAllocator() function and trust the programmer to use it correctly. If it is called when the Lua state was created without a Zig allocator, it will fail

2. Update the "wrap" functions to always set the allocator in the Lua struct. But guard this behind a compile flag that is enabled by default. If the compile flag is disabled, then the allocator is not set in the "wrap" functions. This opens up the user to use the newState, newStateLibc, and setAllocF functions without issues.

3. Add some sort of state tracking in the Lua struct to determine if the data pointer is a Zig allocator. This seems messy but may be possible.

4. (probably not a good idea) Remove the newState, newStateLibc, and setAllocF functions entirely, which removes the issues listed above. And then update the "wrap" functions to always set the allocator in the Lua struct.

[nurpax]

1. I guess I wouldn't mind calling lua.getAllocator(). Also if the binding code doesn't need an allocator, no code gets generated to set it up in the binding glue code.

2. What if you drop setAllocF and allow the Allocator to be set only through the state init functions? I'm not very familiar with Lua engines in general but just wondering if the setAllocF in the Zig API is there just because the Lua C API does it that way, but there's no practical reason for it to exist..? At least it seems a little dangerous to allow setting the allocfn multiple times. This might lead to objects being allocated using allocator A, and later deallocated using allocator B. Or if Lua objects with userdata get __gc'd, are they going to use the original allocator A to free memory?

I'm not sure how newStateLibc differs from setting up a Lua state with the c_allocator? I guess this can be a useful configuration in the sense that all lua allocs are guaranteed to be normal malloc/free/realloc without any Zig business in between.

[natecraddock]

Thinking about this more, I am leaning towards a get function rather than directly accessing lua.allocator. Maybe even removing the lua.allocator field entirely.

I'm not very familiar with Lua engines in general but just wondering if the setAllocF in the Zig API is there just because the Lua C API does it that way, but there's no practical reason for it to exist..? At least it seems a little dangerous to allow setting the allocfn multiple times. This might lead to objects being allocated using allocator A, and later deallocated using allocator B. Or if Lua objects with userdata get __gc'd, are they going to use the original allocator A to free memory?

I had not considered that case, and it does appear setAllocF can be dangerous if misused. After thinking for a while, I think one possible use case would be to temporarily swap out the alloc function (but not the backing allocator) to analyze the allocations? I could imagine some C code might want to swap out the allocator occasionally to track allocation information. There may be other uses.

So even in C this seems like the kind of code where you need to trust the programmer to do the right thing.

---

Another use case of Ziglua is to create shared compiled modules to be loaded into other Lua runtimes with require. In those cases there isn't a Zig allocator in the Lua state. This leads me to think a function to get the allocator is the best way forward.

---

All of this reminds me of something related that I've been thinking about over the last year. Currently Ziglua accepts a std.mem.Allocator and then allocates memory for an Allocator pointer. This is to ensure the alloc function passed to Lua has a stable pointer to the allocator. I copied this design from Zoltan (now unmaintained) https://github.com/ranciere/zoltan.

So far this has worked fine, but I have wondered if it would be better to accept an allocator pointer in the init function. This seems to be less conventional in Zig, but the current code to store a *Allocator in the Lua struct is also unusual. I've tested this, and I like it so far:

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    var lua = try Lua.init(&allocator);
    defer lua.deinit();

    // ...

So long as the allocator that was used to create the Lua state stays in scope, this code would be safe. In other words, the programmer is now tasked with ensuring the allocator is available throughout the duration of the Lua state lifetime.

I like this. It simplifies the Ziglua implementation slightly, and also opens things up for other simplifications in Ziglua (like making the Lua struct an opaque type instead).

So my current plan is this:

1. Add a new function Lua.allocator() that returns the Zig allocator used to initialize the Lua state. If the state was not created with a Zig allocator, or setAllocF modified the state without a Zig allocator, this will cause issues.
2. Change the init function to accept an allocator pointer

Then in the future

3. Update the Lua state to be an opaque {}.
4. Consider removing setAllocF, newState, and newStateLibc. I know GitHub isn't a perfect sample, but I couldn't find a single use of these from any users of ziglua. With the allocator field gone, these are still useful and there wouldn't be any issues with the wrap functions. So we could also not remove them.

I'll let these ideas sit for a few more days, then I will implement them unless we find issues in this plan.

[nurpax]

My initial reaction is that storing a pointer to an Allocator is dangerous and that it's a code smell. Zig being the glass cannon that it is, IMO it makes it very easy to take the address of a temporary leading to hard to debug dangling pointer bugs. F.ex.

fn someFunc() {
    const obj = SomeStruct{
        .allocator = &gpa.allocator() // <-- dangling ptr bug right there
    }
    return obj;
}

Why does the Allocator need to be a pointer in the first place? In Zig's std lib for example it's without exception passed by value. It's a small struct, basically just a virtual function table. I'd much rather store it as an optional ?Allocator rather than a pointer that very easily becomes a dangling pointer.

[natecraddock]

My initial reaction is that storing a pointer to an Allocator is dangerous and that it's a code smell.

I agree. I guess in the case of initializing a Lua state, I would hope a dangling pointer bug surfaces early and loud.

Why does the Allocator need to be a pointer in the first place?

Lua supports custom allocators through the lua_newstate function, which accepts a lua_Alloc function pointer for the custom allocator, and a void *ud user data. The ud pointer is passed to the allocator function. The alloc function needs some way to allocate, either through some global allocator (like malloc()) or through data passed in the void *ud.

So, unless I'm mistaken, the only way to get a Zig allocator into this alloc function is through a pointer. The only question at this point is if the Lua state should hide the pointer internally (as is done currently) or make it explicit at the call site.

[nurpax]

I think I didn't get the benefit of you wanted to do this.. But maybe I do now. So you'd like to store the ptr in the C Lua state structure and if Zig code needs it, cast it to an *Allocator from void*?

I guess I'd be in support of something like this!

[natecraddock]

Haha maybe I haven't been clear enough, since it seems like there is a disconnect. Let me be more detailed here.

Current behavior

A Lua state is passed an Allocator (not a pointer)

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    var lua = try Lua.init(allocator);
    defer lua.deinit();

    // ...

In the init code, an Allocator pointer is created. The provided allocator is copied to the pointer.

The Allocator pointer (allocator_ptr) is passed to lua_newstate. So Lua is now storing a pointer to the allocator.

/// Initialize a Lua state with the given allocator
pub fn init(allocator: Allocator) !Lua {
    // the userdata passed to alloc needs to be a pointer with a consistent address
    // so we allocate an Allocator struct to hold a copy of the allocator's data
    const allocator_ptr = allocator.create(Allocator) catch return error.Memory;
    allocator_ptr.* = allocator;

    const state = c.lua_newstate(alloc, allocator_ptr) orelse return error.Memory;
    return Lua{
        .allocator = allocator_ptr,
        .state = state,
    };
}

So currently Lua already uses the Zig allocator for all internal allocations.

Proposed behavior

A Lua state is passed an Allocator pointer

pub fn main() anyerror!void {
    var gpa = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = gpa.allocator();
    defer _ = gpa.deinit();

    var lua = try Lua.init(&allocator);
    defer lua.deinit();

    // ...

The allocator pointer is passed directly to Lua. constCast is safe here because the pointer is never mutated inside of Lua (the lua_newstate parameter could be const).

/// Initialize a Lua state with the given allocator
pub fn init(allocator: *const Allocator) !Lua {
    const state = c.lua_newstate(alloc, @constCast(allocator)) orelse return error.Memory;
    return Lua{.state = state};
}

So in both the current and proposed implementation, Lua already has a pointer to a Zig allocator. This is already used for all internal Lua allocations. The only difference is how the pointer is created.

I like the proposed solution because it makes it clear that Ziglua/Lua relies on a pointer to an allocator, rather than hiding that unconventional use.

In hindsight, maybe bringing up this in this issue was a bad idea because it isn't 100% related to the main topic...

[nurpax]

OK, makes sense. I'll mention it here for completeness since the below style of initialization is not idiomatic either.

To me an alternative would be to consider that it's the Lua object that needs to exist through a stable pointer. Then this style of initialization would be possible:

/// Initialize a Lua state with the given allocator
pub fn init(lua:*Lua, allocator: Allocator) !void {
    lua.allocator = allocator;
    lua.state = c.lua_newstate(alloc, @constCast(&lua.allocator)) orelse return error.Memory;
}

I fully realize this is not the usual way to initialize new structs in Zig. I guess this style of init() is what's called the "placement new()" in C++.

[natecraddock]

Thanks for mentioning this alternative. While it is not the typical "init" pattern in Zig, it is good to consider.

The biggest downside I see here (other than the more clunky initialization), is that it keeps the lua.allocator field. I want to remove that field in favor of a lua.allocator() function. That field is what causes a lot of the confusion raised in this thread.

However, if we find in the future that the pointer is confusing or causes issues, I would consider this as the first alternative. So thanks again for sharing this approach!