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

Crash in InternPool when faulty packed struct is inside an extern struct #17859

Open
IntegratedQuantum opened this issue Nov 4, 2023 · 4 comments
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone

Comments

@IntegratedQuantum
Copy link
Contributor

Zig Version

0.12.0-dev.1390+94cee4fb2

Steps to Reproduce and Observed Behavior

pub const ChunkMesh = extern struct {
	position: packed struct(u32) {
		vals: u42, // Wrong number of bits
	},
	blockAndModel: u32,
};

fn getBlockFromRenderThread() u32 {
	var mesh: *ChunkMesh = undefined;
	return mesh.chunk.getBlock();
}

pub fn updateAndGetRenderChunks() []*ChunkMesh {
	return undefined;
}

pub fn main() !void {
	_ = getBlockFromRenderThread();
	_ = updateAndGetRenderChunks();
}
$ zig build-exe test.zig
Segmentation fault (core dumped)
Trace of a debug build of the compiler
$ zig-debug build-exe test.zig
thread 9369 panic: reached unreachable code
Analyzing test.zig: test.zig:getBlockFromRenderThread
      %10 = dbg_block_begin()
      %11 = dbg_stmt(2, 2)
      %12 = block_comptime({
        %13 = decl_val("ChunkMesh") token_offset:9:13 to :9:22
        %14 = ptr_type(%13, One) node_offset:9:12 to :9:22
        %15 = break(%12, %14)
      }) node_offset:9:12 to :9:22
    > %16 = alloc_mut(%12) node_offset:9:2 to :9:34
      %17 = store_node(%16, @undef) node_offset:9:25 to :9:34
      %18 = dbg_var_ptr(%16, "mesh")
      %19 = dbg_stmt(3, 2)
      %20 = ret_type() node_offset:10:2 to :10:30
      %21 = dbg_stmt(3, 13)
      %22 = field_ptr(%16, "chunk") node_offset:10:9 to :10:19
      %23 = dbg_stmt(3, 19)
      %24 = dbg_stmt(3, 28)
      %25 = field_call(.auto, %22, "getBlock", []) node_offset:10:9 to :10:30
      %26 = as_node(%20, %25) node_offset:10:9 to :10:30
      %27 = dbg_stmt(3, 2)
      %28 = restore_err_ret_index(%26)
      %29 = ret_node(%26) node_offset:10:2 to :10:30
      %30 = dbg_block_end()
      %31 = restore_err_ret_index(%9)
      %32 = break(%9, @void_value)
    For full context, use the command
      zig ast-check -t test.zig

  in test.zig: test.zig:getBlockFromRenderThread
    > %9 = block({%10..%32}) node_offset:8:35 to :8:35

/home/mint/zig/lib/std/debug.zig:342:14: 0x8cf8a5c in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/mint/zig/src/InternPool.zig:2053:15: 0x8e69743 in toType (zig)
        assert(i != .none);
              ^
/home/mint/zig/src/type.zig:2646:56: 0x937dc96 in comptimeOnlyAdvanced (zig)
                                if (try field_ty.toType().comptimeOnlyAdvanced(mod, opt_sema)) {
                                                       ^
/home/mint/zig/src/type.zig:2555:69: 0x937d1da in comptimeOnlyAdvanced (zig)
                        else => return child_ty.comptimeOnlyAdvanced(mod, opt_sema),
                                                                    ^
/home/mint/zig/src/Sema.zig:37067:35: 0x90cefd0 in typeRequiresComptime (zig)
    return ty.comptimeOnlyAdvanced(sema.mod, sema);
                                  ^
/home/mint/zig/src/Sema.zig:25316:39: 0x9c2ab9a in validateVarType (zig)
    if (!try sema.typeRequiresComptime(var_ty)) return;
                                      ^
/home/mint/zig/src/Sema.zig:3943:29: 0x96ae31a in zirAllocMut (zig)
    try sema.validateVarType(block, ty_src, var_ty, false);
                            ^
/home/mint/zig/src/Sema.zig:1007:66: 0x9365b35 in analyzeBodyInner (zig)
            .alloc_mut                    => try sema.zirAllocMut(block, inst),
                                                                 ^
/home/mint/zig/src/Sema.zig:5853:34: 0x9d0d29b in resolveBlockBody (zig)
        if (sema.analyzeBodyInner(child_block, body)) |_| {
                                 ^
/home/mint/zig/src/Sema.zig:5836:33: 0x9784fd8 in zirBlock (zig)
    return sema.resolveBlockBody(parent_block, src, &child_block, body, inst, &label.merges);
                                ^
/home/mint/zig/src/Sema.zig:1577:49: 0x937734a in analyzeBodyInner (zig)
                    break :blk try sema.zirBlock(block, inst, tags[@intFromEnum(inst)] == .block_comptime);
                                                ^
/home/mint/zig/src/Sema.zig:916:30: 0x9652178 in analyzeBody (zig)
    _ = sema.analyzeBodyInner(block, body) catch |err| switch (err) {
                             ^
/home/mint/zig/src/Module.zig:4707:21: 0x9344d86 in analyzeFnBody (zig)
    sema.analyzeBody(&inner_block, fn_info.body) catch |err| switch (err) {
                    ^
/home/mint/zig/src/Module.zig:3357:40: 0x90c856d in ensureFuncBodyAnalyzed (zig)
            var air = mod.analyzeFnBody(func_index, sema_arena) catch |err| switch (err) {
                                       ^
/home/mint/zig/src/Compilation.zig:3519:42: 0x90c675d in processOneJob (zig)
            module.ensureFuncBodyAnalyzed(func) catch |err| switch (err) {
                                         ^
/home/mint/zig/src/Compilation.zig:3456:30: 0x8e9a84e in performAllTheWork (zig)
            try processOneJob(comp, work_item, main_progress_node);
                             ^
/home/mint/zig/src/Compilation.zig:2237:31: 0x8e95ecc in update (zig)
    try comp.performAllTheWork(main_progress_node);
                              ^
/home/mint/zig/src/main.zig:4236:24: 0x8ec5452 in updateModule (zig)
        try comp.update(main_progress_node);
                       ^
/home/mint/zig/src/main.zig:3637:17: 0x8ee78f2 in buildOutputType (zig)
    updateModule(comp) catch |err| switch (err) {
                ^
/home/mint/zig/src/main.zig:279:31: 0x8cfabff in mainArgs (zig)
        return buildOutputType(gpa, arena, args, .{ .build = .Exe });
                              ^
/home/mint/zig/src/main.zig:223:20: 0x8cf7ee5 in main (zig)
    return mainArgs(gpa, arena, args);
                   ^
/home/mint/zig/lib/std/start.zig:581:37: 0x8cf78fe in main (zig)
            const result = root.main() catch |err| {
                                    ^
../sysdeps/nptl/libc_start_call_main.h:58:16: 0x7f3e34b22d8f in __libc_start_call_main (../sysdeps/x86/libc-start.c)
../csu/libc-start.c:392:3: 0x7f3e34b22e3f in __libc_start_main_impl (../sysdeps/x86/libc-start.c)
???:?:?: 0x40d2824 in ??? (???)
???:?:?: 0x0 in ??? (???)
Aborted (core dumped)

Expected Behavior

The compiler should correctly report the error:

test.zig:2:26: error: backing integer type 'u32' has bit size 32 but the struct fields have a total bit size of 42
 position: packed struct(u32) {
                         ^~~
@IntegratedQuantum IntegratedQuantum added the bug Observed behavior contradicts documented or intended behavior label Nov 4, 2023
@mlugg
Copy link
Member

mlugg commented Nov 6, 2023

Caused by struct_ty.haveFieldTypes() returning true after field types were only partially resolved. Fix relatively simple, but waiting until #17692 since it would conflict.

@kcbanner
Copy link
Contributor

        pub fn haveFieldTypes(s: @This(), ip: *const InternPool) bool {
            const types = s.field_types.get(ip);
-           return types.len == 0 or types[0] != .none;
+           if (types.len == 0) return true;
+           if (types[types.len - 1] != .none) return s.layout == .Packed or !s.flagsPtr(ip).field_types_wip;
+           return false;
        }

I was working on a fix for this - the above diff prevents the crash but then there is an incorrect error on the 2nd analysis:

17859.zig:2:2: error: duplicate struct field: 'position'

To trigger this you need ChunkMesh to be in the file scope, and trigger two analysis of it. The first analysis fails trying to resolve the backing int type (as it should), but the partially analyzed struct still exists, hence the duplicate struct field false positive.

Is this 2nd analysis supposed to be prevented by the switch (mod.declPtr(owner_decl).analysis) { block in resolveTypeFieldsStruct?

@IntegratedQuantum
Copy link
Contributor Author

It seems to work correct now as of 0.13.0

@nektro
Copy link
Contributor

nektro commented Jul 5, 2024

we should double check there's a test covering this before closing

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
Development

No branches or pull requests

5 participants