Skip to content

LLVM backend hits unreachable lowering pointer to comptime field #12963

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

Closed
omaraaa opened this issue Sep 25, 2022 · 16 comments
Closed

LLVM backend hits unreachable lowering pointer to comptime field #12963

omaraaa opened this issue Sep 25, 2022 · 16 comments
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone

Comments

@omaraaa
Copy link
Contributor

omaraaa commented Sep 25, 2022

Zig Version

0.10.0-dev.4060+61aaef0b0

Steps to Reproduce

const std = @import("std");

test {
    const T = std.meta.Tuple(&.{struct {}});
    var t: T = undefined;
    inline for (std.meta.fields(T)) |f| {
        var ptr = &@field(t, f.name);
        _ = ptr;
    }
}

Expected Behavior

All 1 tests passed.

Actual Behavior

error: TODO (LLVM): implement const of pointer type '[TODO fix internal compiler bug regarding dump]' (value.Value.Tag.comptime_field_ptr)
@omaraaa omaraaa added the bug Observed behavior contradicts documented or intended behavior label Sep 25, 2022
@nektro
Copy link
Contributor

nektro commented Sep 25, 2022

expected behavior is multiple compile errors

@omaraaa
Copy link
Contributor Author

omaraaa commented Sep 25, 2022

expected behavior is multiple compile errors

What would the compile errors be? The code compiles and passes in stage 1.

@omaraaa
Copy link
Contributor Author

omaraaa commented Sep 26, 2022

Another variation:

const std = @import("std");

test {
    const T = std.meta.Tuple(&[_]type{struct {}});
    var t: T = undefined;
    var ptr = &t.@"0";
    _ = ptr;
}

It seems to me that getting a pointer to a zero-sized field is the problem.

@Vexu Vexu added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Sep 26, 2022
@Vexu Vexu added this to the 0.10.0 milestone Sep 26, 2022
@Vexu
Copy link
Member

Vexu commented Sep 26, 2022

This is not specific to tuples:

const std = @import("std");

test {
    const T = struct {
        comptime a: struct{} = .{},
    };
    var t: T = undefined;
    inline for (std.meta.fields(T)) |f| {
        var ptr = &@field(t, f.name);
        _ = ptr;
    }
}

@omaraaa
Copy link
Contributor Author

omaraaa commented Sep 26, 2022

This is not specific to tuples:

const std = @import("std");

test {
    const T = struct {
        comptime a: struct{} = .{},
    };
    var t: T = undefined;
    inline for (std.meta.fields(T)) |f| {
        var ptr = &@field(t, f.name);
        _ = ptr;
    }
}

This seems like a separate issue that is hitting the same TODO. It should produce a compile error that you can't use T in a runtime context...but it doesn't.

The main issue with tuples/anonStructs is that the logic for zero-sized field pointers in sema is flawed. In tupleFieldPtr:

  • It checks if the value of a field is comptime.
  • Zero-sized fields are comptime since they have zero runtime bits
  • Thus it outputs a comptime_field_ptr for zero-sized field pointers!

#12977 fixed tuples/@Type zero-sized field pointers for me.

@Vexu
Copy link
Member

Vexu commented Sep 26, 2022

It should produce a compile error that you can't use T in a runtime context...but it doesn't.

Sure you can, all the operations on them just end up being comptime known since they can only have one possible value.

  • Zero-sized fields are comptime since they have zero runtime bits

Zero-sized fields are comptime since they have only one possible value which is know at comptime.

I think the correct solution is to generate comptime_field_ptrs as undefined pointers.

@omaraaa
Copy link
Contributor Author

omaraaa commented Sep 26, 2022

Zero-sized fields are comptime since they have only one possible value which is know at comptime.

Yes, for values. But shouldn't a pointer to field have a runtime value, even if it's zero-sized?

const Foo = struct {
  a: struct{} = .{},
};

// This is fine in stage1 and stage2, no comptime_field_ptr
var foo: Foo = .{};
var ptr = &foo.a;
_ = ptr;

//This doesn't work in stage2, produces comptime_field_ptr
const T = std.meta.Tuple(&[_]type{struct {}});
var t: T = .{ .@"0" = .{} };
var ptr2 = &t.@"0";
_ = ptr2;

Both are essentially the same, but the first example works while the other doesn't.

edit: I suspect that:

  • In 'structFieldPtrByIndex`, 'field.is_comptime' is used to check if the field is comptime.
    • field.is_comptime returns false for zero-sized fields that are not comptime.
  • While in tupleFieldPtr, field_val.tag() != .unreachable_value is used.
    • field_val.tag() != .unreachable_value returns true for zero-sized fields, since a zero-sized field is knowable in comptime.

I think the correct solution is to generate comptime_field_ptrs as undefined pointers.

From what I understood, comptime_field_ptrs should never reach llvm. If is does then there is something wrong in sema.
But I'm not familiar with Zig's codebase, so I might be wrong.

@Vexu
Copy link
Member

Vexu commented Sep 26, 2022

This is also not specific to zero-bit types:

test {
    const T = struct {
        comptime a: u32 = 2,
    };
    var t: T = undefined;
    var ptr = &t.a;
    _ = ptr;
}

From what I understood, comptime_field_ptrs should never reach llvm.

Pointers to comptime fields should behave same as pointers to container-level variables so that above example should behave same as:

test {
    const T = struct {
        var a: u32 = 2;
    };
    var ptr = &T.a;
    _ = ptr;
}

@omaraaa
Copy link
Contributor Author

omaraaa commented Sep 27, 2022

Oh, so stage2 allows structs with comptime fields to be a runtime variable. Didn't know that.

I think the correct solution is to generate comptime_field_ptrs as undefined pointers.

Will this mean that @fieldParentPtr will not work for zero-sized fields?

@Vexu
Copy link
Member

Vexu commented Sep 28, 2022

Oh, so stage2 allows structs with comptime fields to be a runtime variable. Didn't know that.

That has always been allowed, I think you might be confusing comptime fields with fields with comptime only types:

const S = struct {
    // comptime field
    comptime a: i32 = 2,
    // field with a comptime only type
    b: comptime_int,
};

The key difference is that comptime fields are tied to the type while fields with comptime only types are tied to an instance.

Will this mean that @fieldParentPtr will not work for zero-sized fields?

No, but it does mean that @fieldParentPtr should disallow comptime fields which is a missing compile error, and that for tuples you'll have to construct the type using @Type since the regular syntax does not support making runtime zero-bit fields.

@Jarred-Sumner
Copy link
Contributor

Jarred-Sumner commented Dec 25, 2022

Running into this in Bun

The code causing the issue:

else if (comptime strings.eqlComptime(function_name_literal, "hasProperty")) {
    def.hasProperty = @field(staticFunctions, "hasProperty").rfn;
} else if (comptime strings.eqlComptime(function_name_literal, "getProperty")) {
    def.getProperty = @field(staticFunctions, "getProperty").rfn;
} else if (comptime strings.eqlComptime(function_name_literal, "setProperty")) {
    def.setProperty = @field(staticFunctions, "setProperty").rfn;
} else if (comptime strings.eqlComptime(function_name_literal, "deleteProperty")) {
    def.deleteProperty = &@field(staticFunctions, "deleteProperty").rfn;
} else if (comptime strings.eqlComptime(function_name_literal, "getPropertyNames")) {
    def.getPropertyNames = @field(staticFunctions, "getPropertyNames").rfn;
} else if (comptime strings.eqlComptime(function_name_literal, "convertToType")) {
    def.convertToType = @field(staticFunctions, "convertToType").rfn;

Workaround: make rfn a const ptr

Avokadoen added a commit to Avokadoen/ecez that referenced this issue Jan 5, 2023
This commit resolves issue 'Resolve issue with zig self-hosted' (#32).

The only pain point is ziglang/zig#12963
which forces us to predefine some types for initial state of entities.
Avokadoen added a commit to Avokadoen/ecez that referenced this issue Jan 5, 2023
This commit resolves issue 'Resolve issue with zig self-hosted' (#32).

The only pain point is ziglang/zig#12963
which forces us to predefine some types for initial state of entities.
@andrewrk andrewrk modified the milestones: 0.10.1, 0.11.0 Jan 10, 2023
@omaraaa
Copy link
Contributor Author

omaraaa commented Jun 16, 2023

Hi, just an update on this issue.

I tested this issue again on version 0.11.0-dev.3654+2d6d2a1d1 and the TODO is now gone; however, now Zig just crashes without any errors.

I also tried this code:

const std = @import("std");

test {
    const T = struct {struct {}};
    var t: T = undefined;
    inline for (std.meta.fields(T)) |f| {
        var ptr = &@field(t, f.name);
        _ = ptr;
    }
}

Same thing. The compiler just crashes.

Edit:

This crashes too.

test {
    const T = struct { struct {} };
    var t: T = undefined;
    var ptr = &t[0];
    _ = ptr;
}

@andrewrk
Copy link
Member

Current status as of 0.11.0-dev.4173+8924f81d8:

thread 727898 panic: reached unreachable code
/home/andy/Downloads/zig/src/codegen/llvm.zig:3697:40: 0x5cbbd28 in lowerValue (zig)
                    .comptime_field => unreachable,
                                       ^
/home/andy/Downloads/zig/src/codegen/llvm.zig:4650:42: 0x665dfe8 in resolveValue (zig)
        const llvm_val = try o.lowerValue(tv.val.toIntern());
                                         ^
/home/andy/Downloads/zig/src/codegen/llvm.zig:4639:47: 0x665dca0 in resolveInst (zig)
        const llvm_val = try self.resolveValue(.{
                                              ^
/home/andy/Downloads/zig/src/codegen/llvm.zig:8612:49: 0x66a03ff in airStore (zig)
        const src_operand = try self.resolveInst(bin_op.rhs);
                                                ^
/home/andy/Downloads/zig/src/codegen/llvm.zig:4838:53: 0x6120bac in genBody (zig)
                .store          => try self.airStore(inst, false),
                                                    ^
/home/andy/Downloads/zig/src/codegen/llvm.zig:1543:19: 0x611aa05 in updateFunc (zig)
        fg.genBody(air.getMainBody()) catch |err| switch (err) {
                  ^
/home/andy/Downloads/zig/src/link/Elf.zig:2583:74: 0x6125289 in updateFunc (zig)
        if (self.llvm_object) |llvm_object| return llvm_object.updateFunc(mod, func_index, air, liveness);
                                                                         ^
/home/andy/Downloads/zig/src/link.zig:575:77: 0x5e8e993 in updateFunc (zig)
            .elf   => return @fieldParentPtr(Elf,   "base", base).updateFunc(module, func_index, air, liveness),
                                                                            ^
/home/andy/Downloads/zig/src/Module.zig:3988:37: 0x5c73e02 in ensureFuncBodyAnalyzed (zig)
            comp.bin_file.updateFunc(mod, func_index, air, liveness) catch |err| switch (err) {
                                    ^
/home/andy/Downloads/zig/src/Compilation.zig:3137:42: 0x5c714b0 in processOneJob (zig)
            module.ensureFuncBodyAnalyzed(func) catch |err| switch (err) {
                                         ^
/home/andy/Downloads/zig/src/Compilation.zig:3074:30: 0x5ad3973 in performAllTheWork (zig)
            try processOneJob(comp, work_item, main_progress_node);
                             ^
/home/andy/Downloads/zig/src/Compilation.zig:2028:31: 0x5acf6bb in update (zig)
    try comp.performAllTheWork(main_progress_node);
                              ^
/home/andy/Downloads/zig/src/main.zig:3870:24: 0x5afed6e in updateModule (zig)
        try comp.update(main_progress_node);
                       ^
/home/andy/Downloads/zig/src/main.zig:3305:17: 0x5b2e80b in buildOutputType (zig)
    updateModule(comp) catch |err| switch (err) {
                ^
/home/andy/Downloads/zig/src/main.zig:275:31: 0x598ea44 in mainArgs (zig)
        return buildOutputType(gpa, arena, args, .zig_test);
                              ^
/home/andy/Downloads/zig/src/main.zig:213:20: 0x598bbc5 in main (zig)
    return mainArgs(gpa, arena, args);
                   ^
/home/andy/Downloads/zig/lib/std/start.zig:608:37: 0x598b61e in main (zig)
            const result = root.main() catch |err| {
                                    ^
???:?:?: 0x7fb453f6b24d in ??? (libc.so.6)
Unwind information for `libc.so.6:0x7fb453f6b24d` was not available (error.MissingDebugInfo), trace may be incomplete

???:?:?: 0x7ffd8d1e78df in ??? (???)
Aborted (core dumped)

@andrewrk andrewrk modified the milestones: 0.11.0, 0.11.1 Jul 22, 2023
@Vexu Vexu changed the title stage3: error: TODO (LLVM): implement const of pointer type '[TODO fix internal compiler bug regarding dump]' (value.Value.Tag.comptime_field_ptr) LLVM backend hits unreachable lowering pointer to comptime field Nov 8, 2023
@perillo
Copy link
Contributor

perillo commented Jan 30, 2024

Interesting that the compiler crashes with terminated by signal SIGSEGV (Address boundary error) when using llvm backend and terminated by signal SIGTRAP (Trace or breakpoint trap) when using x86_64 and c backends.

@Vexu
Copy link
Member

Vexu commented Jan 30, 2024

Both still hit unreachable with a debug build.

@Vexu
Copy link
Member

Vexu commented Mar 26, 2024

This is now an error:

a.zig:13:19: error: runtime value contains reference to comptime var
        var ptr = &@field(t, f.name);
                  ^~~~~~~~~~~~~~~~~~
a.zig:13:19: note: comptime var pointers are not available at runtime

@Vexu Vexu closed this as completed Mar 26, 2024
@Vexu Vexu modified the milestones: 0.13.0, 0.12.0 Mar 26, 2024
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 frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
6 participants