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

Syntax error in generated code inside zig-cache (translate-c?) #2043

Closed
ghost opened this issue Mar 10, 2019 · 5 comments
Closed

Syntax error in generated code inside zig-cache (translate-c?) #2043

ghost opened this issue Mar 10, 2019 · 5 comments
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. frontend Tokenization, parsing, AstGen, Sema, and Liveness. translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@ghost
Copy link

ghost commented Mar 10, 2019

I'm trying to get my project working with the zig changes from the last day or two and ran into an error. Here is a branch where this error is happening: https://github.com/dbandstra/oxid/commits/not-working

I'm on Ubuntu right now.

/home/dbandstra/oxid/zig-cache/o/DH8YSD9-pmAXMXLFnwJZqo87mh-lr6V49BszWVU-To157k5V3X2S6NNthdcdKFt0/cimport.zig:1256:26: error: expected token '}', found '.'
                        }.?.* = _val;
                         ^

Here's a paste of the relevant part of that file. I marked the bad line with a comment.

pub fn SDL_memset4(dst: ?*c_void, val: Uint32, dwords: usize) void {
    var _n: usize = (dwords +% c_ulong(3)) / c_ulong(4);
    var _p: [*c]Uint32 = @ptrCast([*c]Uint32, dst);
    var _val: Uint32 = val;
    if (dwords == c_ulong(0)) return;
    __switch: {
        __case_0: {
            switch (dwords % c_ulong(4)) {
                c_ulong(0) => break :__case_0,
                c_ulong(3) => break :__case_1,
                c_ulong(2) => break :__case_2,
                c_ulong(1) => break :__case_3,
                else => break :__switch,
            }
        }
        while (true) {
            __case_3: {
                __case_2: {
                    __case_1: {
                        x: {
                            const _ref = &_p;
                            const _tmp = _ref.*;
                            _ref.* +%= 1;
                            break :x _tmp;
                        }.?.* = _val; // <------------------------------------------ here
                    }
                    x: {
                        const _ref = &_p;
                        const _tmp = _ref.*;
                        _ref.* +%= 1;
                        break :x _tmp;
                    }.?.* = _val;
                }
                x: {
                    const _ref = &_p;
                    const _tmp = _ref.*;
                    _ref.* +%= 1;
                    break :x _tmp;
                }.?.* = _val;
            }
            x: {
                const _ref = &_p;
                const _tmp = _ref.*;
                _ref.* +%= 1;
                break :x _tmp;
            }.?.* = _val;
            if (!x: {
                const _ref = &_n;
                _ref.* -%= 1;
                break :x _ref.*;
            }) break;
        }
    }
}

P.S. Zig syntax highlighting in github! Nice!

edit: here's the c code from SDL_stdinc.h:

/* Note that memset() is a byte assignment and this is a 32-bit assignment, so they're not directly equivalent. */
SDL_FORCE_INLINE void SDL_memset4(void *dst, Uint32 val, size_t dwords)
{
#if defined(__GNUC__) && defined(i386)
    int u0, u1, u2;
    __asm__ __volatile__ (
        "cld \n\t"
        "rep ; stosl \n\t"
        : "=&D" (u0), "=&a" (u1), "=&c" (u2)
        : "0" (dst), "1" (val), "2" (SDL_static_cast(Uint32, dwords))
        : "memory"
    );
#else
    size_t _n = (dwords + 3) / 4;
    Uint32 *_p = SDL_static_cast(Uint32 *, dst);
    Uint32 _val = (val);
    if (dwords == 0)
        return;
    switch (dwords % 4)
    {
        case 0: do {    *_p++ = _val;   /* fallthrough */
        case 3:         *_p++ = _val;   /* fallthrough */
        case 2:         *_p++ = _val;   /* fallthrough */
        case 1:         *_p++ = _val;   /* fallthrough */
        } while ( --_n );
    }
#endif
}
@andrewrk andrewrk added this to the 0.4.0 milestone Mar 10, 2019
@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Mar 10, 2019
@andrewrk
Copy link
Member

Oh wow, I didn't know that Zig was successfully translating SDL_memset4 before. That's pretty neat!

That's probably a good candidate to become a regression test of translate-c.

I'm pretty sure this was broken by the C pointers change - which although it caused this regression, it actually is going to result in much cleaner translate-c output, and make a lot of translate-c code easier to implement.

I think this regression can definitely get fixed before 0.4.0 comes out.

@Hejsil
Copy link
Contributor

Hejsil commented Mar 10, 2019

That switch translation also looks very incorrect, but I don't think:

        case 0: do {    *_p++ = _val;   /* fallthrough */
        case 3:         *_p++ = _val;   /* fallthrough */
        case 2:         *_p++ = _val;   /* fallthrough */
        case 1:         *_p++ = _val;   /* fallthrough */
        } while ( --_n );

can be easily be translated to Zig

@andrewrk
Copy link
Member

Yeah it definitely looks wrong. I think it can work in theory though.

@Hejsil
Copy link
Contributor

Hejsil commented Mar 14, 2019

Actually, this broke with the grammar changes in #1685. Basically, we have this rule at stmt level:

Statement
    <- ...
     / LabeledStatement
     ...
     / AssignExpr SEMICOLON

LabeledStatement <- BlockLabel? (Block / LoopStatement)

AssignExpr expands into all other expressions. LabeledStatement is before AssignExpr so that rule is being parsed first, succeeds and then Statement succeeds. Then the parser tries to end the __case_1 block, which then fails the parsing.

The parser parses the grammar correctly, but the new grammar just does not allow this syntax. I can't come up with a solution on top of my head right now.

Edit: Well, a solution would be to make translate-c conform to this new grammar. Idk if we want to put in the work to make x:{}.* = 1; valid at stmt level.

Hejsil added a commit that referenced this issue Mar 15, 2019
andrewrk added a commit that referenced this issue Mar 15, 2019
@andrewrk andrewrk modified the milestones: 0.4.0, 0.5.0 Apr 8, 2019
@andrewrk andrewrk added the translate-c C to Zig source translation feature (@cImport) label Aug 28, 2019
@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Aug 28, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Oct 23, 2019
@andrewrk andrewrk added contributor friendly This issue is limited in scope and/or knowledge of Zig internals. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Jan 2, 2020
@daurnimator
Copy link
Contributor

This now works in master.

const SDL = @cImport({
    @cInclude("/usr/include/SDL2/SDL_stdinc.h");
});

export fn foo(dst: [*]u32, val: u32, n: usize) void {
    SDL.SDL_memset4(dst, val, n);
}

It was translated to:

pub fn SDL_memset4(arg_dst: ?*c_void, arg_val: Uint32, arg_dwords: usize) void {
    var dst = arg_dst;
    var val = arg_val;
    var dwords = arg_dwords;
    var _n: usize = ((dwords +% @bitCast(c_ulong, @as(c_long, @as(c_int, 3)))) / @bitCast(c_ulong, @as(c_long, @as(c_int, 4))));
    var _p: [*c]Uint32 = (@ptrCast([*c]Uint32, @alignCast(@alignOf(Uint32), (dst))));
    var _val: Uint32 = (val);
    if (dwords == @bitCast(c_ulong, @as(c_long, @as(c_int, 0)))) return;
    __switch: {
        __case_3: {
            __case_2: {
                __case_1: {
                    __case_0: {
                        switch ((dwords % @bitCast(c_ulong, @as(c_long, @as(c_int, 4))))) {
                            @bitCast(c_ulong, @as(c_long, 0)) => break :__case_0,
                            @bitCast(c_ulong, @as(c_long, 3)) => break :__case_1,
                            @bitCast(c_ulong, @as(c_long, 2)) => break :__case_2,
                            @bitCast(c_ulong, @as(c_long, 1)) => break :__case_3,
                            else => break :__switch,
                        }
                    }
                    (blk: {
                        const ref = &_p;
                        const tmp = ref.*;
                        ref.* += 1;
                        break :blk tmp;
                    }).?.* = _val;
                }
                (blk: {
                    const ref = &_p;
                    const tmp = ref.*;
                    ref.* += 1;
                    break :blk tmp;
                }).?.* = _val;
            }
            (blk: {
                const ref = &_p;
                const tmp = ref.*;
                ref.* += 1;
                break :blk tmp;
            }).?.* = _val;
        }
        while (true) {
            (blk: {
                const ref = &_p;
                const tmp = ref.*;
                ref.* += 1;
                break :blk tmp;
            }).?.* = _val;
            if (!((blk: {
                const ref = &_n;
                ref.* -%= 1;
                break :blk ref.*;
            }) != 0)) break;
        }
    }
}

@andrewrk andrewrk modified the milestones: 0.7.0, 0.6.0 Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior contributor friendly This issue is limited in scope and/or knowledge of Zig internals. frontend Tokenization, parsing, AstGen, Sema, and Liveness. translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

No branches or pull requests

3 participants