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

Imported C Structs with VLAs #8759

Closed
iguessthislldo opened this issue May 13, 2021 · 6 comments · Fixed by #8891
Closed

Imported C Structs with VLAs #8759

iguessthislldo opened this issue May 13, 2021 · 6 comments · Fixed by #8891
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@iguessthislldo
Copy link

iguessthislldo commented May 13, 2021

I'm currently using 0.8.0-dev.2192+65d279bc0 if that matters.

Problem:

I'd like to use these C structs from ACPICA in my Zig code. The following code is generated from it:

// (...)/actypes.h:1382:37: warning: struct demoted to opaque type - has variable length array
pub const struct_acpi_pnp_device_id_list = opaque {};
pub const ACPI_PNP_DEVICE_ID_LIST = struct_acpi_pnp_device_id_list;
pub const struct_acpi_device_info = extern struct {
    // ... fields that aren't important ..
    CompatibleIdList: ACPI_PNP_DEVICE_ID_LIST,
};
pub const ACPI_DEVICE_INFO = struct_acpi_device_info;

The warning seemed fair by itself when I first looked at this, but this apparently means that I can't use ACPI_DEVICE_INFO/struct_acpi_device_info at all:

./zig-cache/o/3016e146699c4abaeedfebe9c82141cf/cimport.zig:153:5: error: opaque types have unknown size and therefore cannot be directly embedded in structs
    CompatibleIdList: ACPI_PNP_DEVICE_ID_LIST,
    ^

Having a variable amount of data at the end of a data structure seems like a common pattern in data structures dealing with hardware. I've had to write code like this here for USB EHCI, among other places to access this kind of data from hardware. I'm fine writing code like that and it seems like I'm not the first person writing Zig to do so: #6008.

Possible Solutions?

I'm wondering if, for the sake of better compatibility with C, Zig could only demote the variable length array field itself to an opaque type and allow them at the end of the struct. This would mirror how VLAs act in C. It might also make previously mentioned situation of using this in normal Zig more readable and reduce the amount of pointer arithmetic, but I think that's a minor advantage if it's one at all. Or maybe this should just should be limited to extern structs if it is better to isolate this behavior?

const MyType = extern struct {
  length: usize,
  data: opaque {},
};

Alternatively it would be better from my perspective as a user if Zig fully supported VLAs in structs, but I'm assuming that's not on the table because normal VLAs have been shot down before: #3952.

Also

The actual cimport.zig lines with the warning are:

pub const ACPI_PNP_DEVICE_ID = struct_acpi_pnp_device_id; // /data/development/os/georgios/kernel/platform/acpica/acpica/source/include/actypes.h:1382:37: warning: struct demoted to opaque type - has variable length array
pub const struct_acpi_pnp_device_id_list = opaque {};

It's kinda weird the comment is following ACPI_PNP_DEVICE_ID, shouldn't there be a newline there or it should be following struct_acpi_pnp_device_id_list, which is what it's actually talking about?

@iguessthislldo iguessthislldo changed the title Imported C Structs with a VLA Imported C Structs with VLAs May 13, 2021
@iguessthislldo
Copy link
Author

Also I called them VLAs because that's what Zig called them, but aren't these different? The GCC link I provided able calls them a couple of things, but one of them is "flexible array members", which also seems like what the C standard calls them.

@iguessthislldo
Copy link
Author

Another possible solution is for Zig to ignore the flexible array member.

@SpexGuy SpexGuy added enhancement Solving this issue will likely involve adding new logic or components to the codebase. translate-c C to Zig source translation feature (@cImport) labels May 13, 2021
@ehaas
Copy link
Contributor

ehaas commented May 13, 2021

It's not pretty, but one approach that might work would be, given

typedef struct {
    size_t len;
    int arr[];
} mystruct;

translate to:

pub const mystruct = extern struct {
    len: usize,
};

(i.e. drop the VLA field since @sizeOf needs to return the correct value.)

Then, given mystruct *foo, translate any references to foo->arr to @intToPtr([*c]c_int, @ptrToInt(foo) + @sizeOf(mystruct))

@SpexGuy
Copy link
Contributor

SpexGuy commented May 13, 2021

I think this would be a reasonable translation:

pub const ACPI_PNP_DEVICE_ID_LIST = extern struct {
    Count: u32 align(max(@alignOf(u32), @alignOf(ACPI_PNP_DEVICE_ID)),
    ListSize: u32,
    pub fn Ids(self: *@This()) [*]ACPI_PNP_DEVICE_ID {
        const offset = comptime std.mem.alignForward(@offsetOf(@This(), "ListSize") + @sizeOf(u32), @alignOf(ACPI_PNP_DEVICE_ID));
        return @ptrCast([*]ACPI_PNP_DEVICE_ID, @alignCast(@alignOf(ACPI_PNP_DEVICE_ID), @ptrCast([*]u8, self) + offset));
    }
};

It's not sufficient to just drop the field because the VLA may have higher alignment than the rest of the struct, so that needs to be taken into account.

iguessthislldo added a commit to iguessthislldo/georgios that referenced this issue May 13, 2021
Wait for ziglang/zig#8759 to be resolved to do
anymore work on this?
@ifreund
Copy link
Member

ifreund commented May 16, 2021

Technically a duplicate of #4775 but lets keep this one open instead since the discussion is here.

iguessthislldo added a commit to iguessthislldo/georgios that referenced this issue May 16, 2021
Wait for ziglang/zig#8759 to be resolved to do
anymore work on this?
@andrewrk andrewrk added this to the 0.10.0 milestone May 19, 2021
ehaas added a commit to ehaas/zig that referenced this issue May 24, 2021
ehaas added a commit to ehaas/zig that referenced this issue Jun 10, 2021
Vexu pushed a commit that referenced this issue Jun 11, 2021
@Vexu Vexu modified the milestones: 0.10.0, 0.9.0 Jun 13, 2021
@rohlem
Copy link
Contributor

rohlem commented Jul 15, 2024

Somewhat related, I was wondering whether an empty array member might work for representing this.

const S = extern struct {
  data: u8,
  flexible_array_member: [0]u64,
};

The answer is "kind of": The alignment of S is raised to consider the alignment of the array's element type, and @offsetOf correctly returns 8 here.
For regular struct the alignment is also increased correctly, however automatic field reordering actually puts the flexible array member at offset 0 (overlapping the other field(s)).

For extern struct the alignment increase is useful for modeling this, but for regular struct it's an unnecessary cost to work around in generic code.
I'm not sure whether differing semantics here would be okay or considered too confusing in practice though.

EDIT: My bad, didn't realize the issue was closed. I doubt this quirk would be considered worth its own issue though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants