Skip to content

stage2: Implement explicit backing integers for packed structs #12379

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

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

ifreund
Copy link
Member

@ifreund ifreund commented Aug 8, 2022

Now the backing integer of a packed struct type may be explicitly
specified with e.g. packed struct(u32) { ... }.

I haven't touched Sema.zig this extensively before, so any code/style pointers there are certainly appreciated.

Related: #10113, #5049

@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch from a9c36be to 23f23ac Compare August 8, 2022 23:37
@Jarred-Sumner
Copy link
Contributor

This is exciting

Will probably simplify a lot of code like this:

/// https://nodejs.org/api/os.html#osplatform
pub const OperatingSystem = enum(u16) {
    none = 0,
    all = all_value,

    _,

    pub const aix: u16 = 1 << 1;
    pub const darwin: u16 = 1 << 2;
    pub const freebsd: u16 = 1 << 3;
    pub const linux: u16 = 1 << 4;
    pub const openbsd: u16 = 1 << 5;
    pub const sunos: u16 = 1 << 6;
    pub const win32: u16 = 1 << 7;
    pub const android: u16 = 1 << 8;

    pub const all_value: u16 = aix | darwin | freebsd | linux | openbsd | sunos | win32 | android;

    pub fn isMatch(this: OperatingSystem) bool {
        if (comptime Environment.isLinux) {
            return (@enumToInt(this) & linux) != 0;
        } else if (comptime Environment.isMac) {
            return (@enumToInt(this) & darwin) != 0;
        } else {
            return false;
        }
    }

    const NameMap = ComptimeStringMap(u16, .{
        .{ "aix", aix },
        .{ "darwin", darwin },
        .{ "freebsd", freebsd },
        .{ "linux", linux },
        .{ "openbsd", openbsd },
        .{ "sunos", sunos },
        .{ "win32", win32 },
        .{ "android", android },
    });

    pub fn apply(this_: OperatingSystem, str: []const u8) OperatingSystem {
        if (str.len == 0) {
            return this_;
        }
        const this = @enumToInt(this_);

        const is_not = str[0] == '!';
        const offset: usize = if (str[0] == '!') 1 else 0;

        const field: u16 = NameMap.get(str[offset..]) orelse return this_;

        if (is_not) {
            return @intToEnum(OperatingSystem, this & ~field);
        } else {
            return @intToEnum(OperatingSystem, this | field);
        }
    }
};

@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch from 23f23ac to 5136e82 Compare August 9, 2022 15:40
@ifreund
Copy link
Member Author

ifreund commented Aug 9, 2022

I found some underlying issues with my previous approach and have just pushed an implementation that should be much more correct. The ZIR encoding now handles the case where the type requires a body properly and the layout of packed structs is not force-resolved while resolving their fields.

However, I have run into an issue I'm not sure how to solve. With my changes zig build test-std crashes with the following output:

Semantic Analysis [5743] min... thread 27123 panic: reached unreachable code
Analyzing /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:BytesAsSliceReturnType
      %21196 = decl_ref("meta") 
      %21197 = field_call_bind(%21196, "Child") 
      %21198 = dbg_stmt(6, 66)
      %21199 = param_type(%21197, 0)
      %21200 = call(.compile_time, %21197, [%21136]) 
      %21201 = type_info(%21200) 
      %21202 = field_val(%21201, "Array") 
      %21203 = field_val(%21202, "len") 
      %21204 = size_of(%21134) 
    > %21205 = mod_rem(%21203, %21204) 
      %21206 = cmp_neq(%21205, @Zir.Inst.Ref.zero) 
      %21207 = as_node(@Zir.Inst.Ref.bool_type, %21206) 
      %21208 = break_inline(%21195, %21207)
    For full context, use the command
      zig ast-check -t /home/ifreund/projects/zig/lib/std/mem.zig

  in /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:BytesAsSliceReturnType
    > %21195 = bool_br_and(%21194, {%21196..%21208}
  in /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:BytesAsSliceReturnType
    > %21211 = block({%21185..%21210}) 
  in /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:bytesAsSlice__anon_34119
    > %21253 = call(.compile_time, %21247, [%21245, %21251]) 
  in /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:bytesAsSlice__anon_34119
    > %21301 = func(ret_ty={%21247..%21254}, body={%21255..%21299}) (lbrace=1:97,rbrace=11:1) 
  in /home/ifreund/projects/zig/lib/std/mem.zig: mem.zig:test.bytesAsSlice on a packed struct
    > %21623 = call(.auto, %21619, [%21604, %21607]) 

/home/ifreund/projects/zig/lib/std/debug.zig:281:14: 0x2ec5708 in std.debug.assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/ifreund/projects/zig/src/type.zig:3211:27: 0x30ab2eb in type.Type.abiSizeAdvanced (zig)
                    assert(struct_obj.haveLayout());
                          ^
/home/ifreund/projects/zig/src/type.zig:3148:32: 0x30a165b in type.Type.abiSize (zig)
        return (abiSizeAdvanced(ty, target, .eager) catch unreachable).scalar;
                               ^
/home/ifreund/projects/zig/src/value.zig:1101:37: 0x34e2b21 in value.Value.toBigIntAdvanced (zig)
                const x = ty.abiSize(target);
                                    ^
/home/ifreund/projects/zig/src/value.zig:1058:36: 0x32f692a in value.Value.toBigInt (zig)
        return val.toBigIntAdvanced(space, target, null) catch unreachable;
                                   ^
/home/ifreund/projects/zig/src/value.zig:3492:40: 0x3aaae6f in value.Value.intRemScalar (zig)
        const rhs_bigint = rhs.toBigInt(&rhs_space, target);
                                       ^
/home/ifreund/projects/zig/src/value.zig:3483:28: 0x38da9d2 in value.Value.intRem (zig)
        return intRemScalar(lhs, rhs, allocator, target);
                           ^
/home/ifreund/projects/zig/src/Sema.zig:11766:58: 0x3691416 in Sema.zirModRem (zig)
                    const rem_result = try lhs_val.intRem(rhs_val, resolved_type, sema.arena, target);
                                                         ^
/home/ifreund/projects/zig/src/Sema.zig:890:45: 0x34a60be in Sema.analyzeBodyInner (zig)
            .mod_rem   => try sema.zirModRem(block, inst),
                                            ^
/home/ifreund/projects/zig/src/Sema.zig:620:45: 0x349872c in Sema.analyzeBodyBreak (zig)
    const break_inst = sema.analyzeBodyInner(block, body) catch |err| switch (err) {
                                            ^
/home/ifreund/projects/zig/src/Sema.zig:585:50: 0x38922a0 in Sema.resolveBody (zig)
    const break_data = (try sema.analyzeBodyBreak(block, body)) orelse
                                                 ^
/home/ifreund/projects/zig/src/Sema.zig:14483:32: 0x36335bc in Sema.zirBoolBr (zig)
        return sema.resolveBody(parent_block, body, inst);
                               ^

The size_of(%21134) instruction creates a value with tag .lazy_size as %21134 is a packed struct which has not yet had its layout resolved. However, the current implementation of mod_rem does not try to resolve lazy values but rather asserts that they are already resolved. It seems to me that the fix would be adding a Value.intRemAdvanced() variant that takes a sema "kit" and tries to resolve lazy values. Is this correct?

@andrewrk
Copy link
Member

andrewrk commented Aug 9, 2022

Hmmm, that's a good question. In the interest of managing complexity, I think I would suggest a different approach, based on the observation that the intRem function is only ever called from Sema:

andy@ark ~/D/zig (master)> grep -RI intRem src/
src/value.zig:    pub fn intRem(lhs: Value, rhs: Value, ty: Type, allocator: Allocator, target: Target) !Value {
src/value.zig:                scalar.* = try intRemScalar(lhs.indexVectorlike(i), rhs.indexVectorlike(i), allocator, target);
src/value.zig:        return intRemScalar(lhs, rhs, allocator, target);
src/value.zig:    pub fn intRemScalar(lhs: Value, rhs: Value, allocator: Allocator, target: Target) !Value {
src/Sema.zig:                    const rem_result = try lhs_val.intRem(rhs_val, resolved_type, sema.arena, target);
src/Sema.zig:                        try lhs_val.intRem(rhs_val, resolved_type, sema.arena, target),

The idea being to move this function to Sema, and accept sema, block, src arguments. I made this change to other functions recently and it was a satisfactory approach. This will also allow you to grab the Allocator and Target from Sema instead of passing them explicitly.

@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch from 5136e82 to b1acd4a Compare August 10, 2022 11:46
@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch 2 times, most recently from 1e1e5f7 to 7c9936e Compare August 10, 2022 13:40
@ifreund ifreund requested a review from kristoff-it as a code owner August 10, 2022 13:40
@kristoff-it
Copy link
Member

Thanks for the update to Autodoc!

@ifreund
Copy link
Member Author

ifreund commented Aug 10, 2022

Thanks for the update to Autodoc!

Not breaking it is the least I can do :)

@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch 3 times, most recently from 43411f6 to 1dd36a3 Compare August 10, 2022 17:10
Now the backing integer of a packed struct type may be explicitly
specified with e.g. `packed struct(u32) { ... }`.
@ifreund ifreund force-pushed the packed-struct-explicit-backing-int branch from 1dd36a3 to 0d32b73 Compare August 10, 2022 17:55
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@andrewrk andrewrk merged commit e017889 into ziglang:master Aug 10, 2022
@ifreund ifreund deleted the packed-struct-explicit-backing-int branch August 10, 2022 23:21
TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024
…backing-int

stage2: Implement explicit backing integers for packed structs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants