translate-c: support for bitfields with clang frontend

[thislight]

The Goals

Make translate-c translating bitfields with clang frontend.

• Supports the C structures with only bitfields.
• Supports unnamed zero-bit field syntax.
• Supports MSVC behaviour with only bitfields.
• Supports regular fields mixing with bitfields.
• Automatic tool to port the bitfield support

Implementation Details

The implementation includes two parts:

a) std.zig.c_translation.EmulateBitfieldStruct, a comptime function do the heavy work, transform structs for C ABI;

b) translate-c, emits code to call EmulateBitfieldStruct.

Discovered Limitation

• Nested extern struct in a bitfield struct is unsupported. translate-c: support for bitfields with clang frontend #20896 (comment)
• The fields in the bitfield struct could not refer the struct itself, even in a pointer. translate-c: support for bitfields with clang frontend #20896 (comment)

[thislight]

@ehaas Thanks for reviewing! Now the feature is implemented, could you take a look again?

[Atomk]

Related issue: #1499

[thislight]

The regression for windows has been fixed.

[NefixEstrada]

Using this patch, instead of the error: opaque types have unknown size and therefore cannot be directly embedded in structs, I get the following error instead:

/home/nefix/dev/zig-rdpg/.zig-cache/o/c4b45cff4d6291eecee0c97566e71379/cimport.zig:13112:35: error: extern structs cannot contain fields of type 'u1'
    reprioritize_blocking_assets: u1 = @import("std").mem.zeroes(u1),
                                  ^~
/home/nefix/dev/zig-rdpg/.zig-cache/o/c4b45cff4d6291eecee0c97566e71379/cimport.zig:13112:35: note: only integers with 0, 8, 16, 32, 64 and 128 bits are extern compatible

The generated bit looks as follows:

const struct_unnamed_128 = @import("std").zig.c_translation.EmulateBitfieldStruct(extern struct {
    reprioritize_blocking_assets: u1 = @import("std").mem.zeroes(u1),
    push_preload: u1 = @import("std").mem.zeroes(u1),
    allow_cross_origin_push: u1 = @import("std").mem.zeroes(u1),
    casper: h2o_casper_conf_t = @import("std").mem.zeroes(h2o_casper_conf_t),
}, &([4]type{
    c_uint,
    c_uint,
    c_uint,
    void,
}), .{});

Which corresponds to the following code: https://github.com/h2o/h2o/blob/master/include/h2o.h#L292-L310

Not sure if this is in the scope of this PR or if it's not working as intended.

Thanks! :)

[thislight]

@NefixEstrada oops, I changed the protocol EmulateBitfieldStruct used and forgot to make the translate-c to emit the new code. You can edit the file manually to use packed struct instead of extern struct and see if it works.

I will fix it soon, Thanks for pointing out!

[thislight]

Sending a new patch which fixed the problem that the translate-c emitted code does not match EmulateBitfieldStruct protocol.

[NefixEstrada]

Sadly, I'm getting a different (but I think related) error:

/home/nefix/dev/zig-rdpg/.zig-cache/o/c4b45cff4d6291eecee0c97566e71379/cimport.zig:13115:13: error: packed structs cannot contain fields of type 'cimport.struct_st_h2o_casper_conf_t'
    casper: h2o_casper_conf_t = @import("std").mem.zeroes(h2o_casper_conf_t),
            ^~~~~~~~~~~~~~~~~
/home/nefix/dev/zig-rdpg/.zig-cache/o/c4b45cff4d6291eecee0c97566e71379/cimport.zig:13115:13: note: only packed structs layout are allowed in packed types
/home/nefix/dev/zig-rdpg/.zig-cache/o/c4b45cff4d6291eecee0c97566e71379/cimport.zig:13106:48: note: struct declared here
pub const struct_st_h2o_casper_conf_t = extern struct {
                                        ~~~~~~~^~~~~~

The generated code looks as follows:

pub const struct_st_h2o_casper_conf_t = extern struct {
    capacity_bits: c_uint = @import("std").mem.zeroes(c_uint),
    track_all_types: c_int = @import("std").mem.zeroes(c_int),
};
pub const h2o_casper_conf_t = struct_st_h2o_casper_conf_t;
const struct_unnamed_128 = @import("std").zig.c_translation.EmulateBitfieldStruct(packed struct {
    reprioritize_blocking_assets: u1 = @import("std").mem.zeroes(u1),
    push_preload: u1 = @import("std").mem.zeroes(u1),
    allow_cross_origin_push: u1 = @import("std").mem.zeroes(u1),
    casper: h2o_casper_conf_t = @import("std").mem.zeroes(h2o_casper_conf_t),
}, &([4]type{
    c_uint,
    c_uint,
    c_uint,
    void,
}), .{});

[thislight]

@NefixEstrada I have assume that the packed struct can store any type of data, clearly I was wrong there. That error is about the DSL describing the structure. It might be easy to fix.

The another problem I found there is that the EmulateBitfieldStruct returns in fact a packed struct, so even if the struct be passed into the function, it will fail at the final step. So now I think it might be impossible to describe C struct with bitfields in 1:1 favour as I originally wanted to achieve.

I will try to figure it out, maybe the structure returned from EmulateBitfieldStruct needs changes.

[thislight]

@NefixEstrada After hours of work, I believe this use case could not be supported. Clearly:

1. We can only control the memory layout in packed struct.
2. The compiler controls the memory layout of extern struct and regular struct.

I have tried these ideas:
a) EmulateBitfieldStruct returns a extern struct, bitfields are grouped together. Like:

struct word {
    unsigned b0: 1;
    unsigned b1: 2;
    unsigned f0;
};

The b0 and b1 groups to bitfields_0:

extern struct {
    bitfileds_0: packed struct { b0: u1, b1: u1, ...},
    f0: c_uint,
}

Two problems with this approach:

1. translate-c translates accessing, this approach requires us rendering different accessing based on the record type.
2. We could not control the memory layout of an extern struct. For example, on the x86-64-linux, the end padding of a bitfield group can be stolen for a next field.

b) Flatten the fields into a single extern struct: we could not control the memory layout.

c) Use packed struct to emulate related/all structures: The impact is large, and may break a lot of existing code. I don't want to give up the compiler's built-in feature just for this one case.

I am running out of ideas. I think this use case is impossible to do at the moment. The best I can do is to emit a warning and return an opaque.

[thislight]

The new patch rejects the nested structure use case and emits warning for structures with any bitfield.

[patryk4815]

typedef struct ngx_file_s            ngx_file_t;

struct ngx_file_s {
    intptr_t                (*thread_handler)(ngx_file_t *file);
    unsigned                   directio:1;
};

pub const struct_ngx_file_s = @import("std").zig.c_translation.EmulateBitfieldStruct(struct {
    thread_handler: ?*const fn (?*ngx_file_t) callconv(.C) isize = @import("std").mem.zeroes(?*const fn (?*ngx_file_t) callconv(.C) isize),
    directio: u1 = @import("std").mem.zeroes(u1),
}, &([2]type{
    void,
    c_uint,
}), .{});
pub const ngx_file_t = struct_ngx_file_s;

^ I tried, c-translate nginx headers.

src/a.zig:9:5: error: dependency loop detected
pub const ngx_file_t = struct_ngx_file_s;
~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[thislight]

src/a.zig:9:5: error: dependency loop detected
pub const ngx_file_t = struct_ngx_file_s;
~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@patryk4815 This problem is about the analysis (it even happens in the structures don't use C bit fields).
I think it may be related to #18247 .

[patryk4815]

@thislight I see. It is issue with @typeInfo. :( I think this is not an issue to be solved here.

---

Maybe c-translate should check if this is 'same' struct referencing itself, and replace it with @This()?

pub const ngx_file_s = EmulateBitfieldStruct(struct {
    thread_handler: ?fn (?*ngx_file_t)
});
pub const ngx_file_t = ngx_file_s;

->

pub const ngx_file_s = EmulateBitfieldStruct(struct {
    thread_handler: ?fn (?*@This())
});
pub const ngx_file_t = ngx_file_s;

[thislight]

Maybe c-translate should check if this is 'same' struct referencing itself, and replace it with @This()?

pub const ngx_file_s = EmulateBitfieldStruct(struct {
    thread_handler: ?fn (?*ngx_file_t)
});
pub const ngx_file_t = ngx_file_s;

->

pub const ngx_file_s = EmulateBitfieldStruct(struct {
    thread_handler: ?fn (?*@This())
});
pub const ngx_file_t = ngx_file_s;

@patryk4815 It may not achieve the goal. When generating the final type, the replaced @This() is still needed to be replaced by the generated type..and it's impossible (think: we need to got the type itself before we generating the type).

Our best chance may be to rework the DSL syntax for EmulateBitfieldStruct, avoid to resolve the type when it's a specific type (like pointer).

// c_translation.zig
pub const Bitfiled = struct {
  name: []const u8,
  @"type": type,
  backing_integer: ?type = null,
  is_pointer: bool = false,
};

pub fn EmulateBitfieldStruct(fields: []const Bitfield) type {
  // ...
}

Consider this case:

pub const ngx_file_s = EmulateBitfieldStruct(&.{
  .{ .name = "thread_handler", .@"type" = ?*const fn (?*ngx_file_t) isize, .is_pointer = true },
  .{ .name = "directio", .@"type" = u1, .backing_integer = c_uint },
});
pub const ngx_file_t = ngx_file_s;

When .is_pointer = true, we can layout the field as usize and avoid accessing the type.

My concern is, even we know the compiler resolves types lazily, I don't know exactly if it's lazy in this case.

[thislight]

Our best chance may be to rework the DSL syntax for EmulateBitfieldStruct, avoid to resolve the type when it's a specific type (like pointer).
My concern is, even we know the compiler resolves types lazily, I don't know exactly if it's lazy in this case.

I have done the rewrite, seems that the approach I suggested does not solve this problem. cc @patryk4815 .

[patryk4815]

Probably this topic unfortunately #6709 (comment)

[thislight]

This PR is going to be closed because it's blocked by some key problems.

I have learnt much from this PR and I hope this experience can also help others on this topic. Thanks all who contributes!