fubark / cyber

Fast and concurrent scripting.
https://cyberscript.dev
MIT License
1.23k stars 44 forks source link

Unstable zig package for tooling #86

Open sno2 opened 8 months ago

sno2 commented 8 months ago

It would be great to have an unstable zig package available. A tokenizer/parser and semantic analyzing API would make tooling (e.g. lsp, formatter, documentation generator) fairly straightforward to implement. I would be willing to add a PR for this.

fubark commented 8 months ago

That would be nice to have! I would appreciate a PR even if it's just a barebones api over the zon package (just reading about it now). I'm not sure what kind of endpoints would be needed for an LSP but I'd be happy to add them when the time comes.

To get started, there is currently https://github.com/fubark/cyber/blob/master/src/capi.zig. It's not comprehensive but it can be used as a reference.

sno2 commented 8 months ago

I was more thinking of exposing cyber.zig, or a more minimal Zig file with tokenizer/parser/sema exposed, as a Zig package and marking the API as non-SemVer. I integrated the tokenizer in the C API because I have not yet played around with Zig bindings generation. However, working the entire parser API along with sema.zig's functionality seems like it would require a lot of work.

fubark commented 8 months ago

Yea that's fine too. A lot of the data types do not map to C structs so that would be easier to expose. The only issue is it would require a particular Zig version when writing an extension but it's better than nothing.

sno2 commented 3 months ago

Now that 0.13 is released, we can cleanly use the Zig build system to depend on the upstream sources for mimalloc and tcc.

@fubark Do you know what Git commit the vendored mimalloc is on? I was able to get tcc working, but I don't see mimalloc's hash mentioned anywhere.

fubark commented 3 months ago

I forgot to record the hash so I ended up upgrading mimalloc to latest. Also, I'm not sure 0.13.0 is stable for Cyber. I saw tests fail from what looked like packed struct miscompilations. (Although it's probably fixed on master)

Edit: Mimalloc is now at 03020fbf81541651e24289d2f7033a772a50f480

sno2 commented 3 months ago

Thanks for upgrading mimalloc, and I see what you mean by the failing tests. Here is my branch, although I didn't commit the tcc module migration.

https://github.com/sno2/cyber/tree/upgrade-zig

Definitely some UB happening in the tests, so I'll probably wait until 0.14 releases and try again then.

sno2 commented 3 months ago

I got cyber building on 0.14, but we are still getting the same error on the ARC test.

https://github.com/sno2/cyber/tree/upgrade-zig-0.14

What steps did you take to figure out that this was most likely a packed struct miscompilation? I wonder if explicitly stating the size of the packed struct like packed struct(...) could help.

fubark commented 3 months ago

I got more tests to pass when I removed the packed modifier so I figured it might be related to packed structs. But I also didn't try removing all of them, so I probably need to look at it again.

fubark commented 3 months ago

Here is a MRE:

const std = @import("std");

pub const Foo = packed struct {
    a: u31,
    b: bool,

    pub fn init() Foo {
        return .{ .a = undefined, .b = true };
    }
};

pub fn foo(a: Foo) void {
    std.debug.print("{} {b}\n", .{a.b, @as(u32, @bitCast(a))});
    // Expected "true", and a non zero value
    // Found "false", 0
}

pub fn main() !void {
    const a = Foo.init();
    foo(a);
}

The issue seems to be passing a packed struct where the first field is initialized as undefined.

sno2 commented 3 months ago

Nice find! I have also been looking into this issue this morning although I hadn't been able to deduce that. It seems like using i32 as the backing integer works around the issue:

pub const Foo = packed struct(i32) {
    a: u31,
    b: bool,

    pub fn init() Foo {
        return .{ .a = undefined, .b = true };
    }
};

u32 has the same problems as the unspecified version.

fubark commented 3 months ago

Neither works for me on 0.14.0-dev.839+a931bfada or 0.13.0. I'm on arm64 so perhaps it's also architecture specific issue.

sno2 commented 3 months ago

Ah, sorry seems to be non-deterministic. I should've ran more than 3 times. (Zig 0.13, Windows)

$ zig run foo.zig
true 10000001010110101010010010100000
$ zig run foo.zig
true 10110001000000111010010010100000
$ zig run foo.zig
true 10101111010011011010010010100000
$ zig run foo.zig
true 11101011000100111010010010100000
$ zig run foo.zig
false 110100111001010010010100000