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

Proposal: Rename packed struct to something more representative #12852

Open
ikskuh opened this issue Sep 15, 2022 · 29 comments
Open

Proposal: Rename packed struct to something more representative #12852

ikskuh opened this issue Sep 15, 2022 · 29 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@ikskuh
Copy link
Contributor

ikskuh commented Sep 15, 2022

As Zig is a lot about communication, we should probably rename packed struct into something that represents more what it actually is.

Right now, a lot of people assume that packed struct follows similar rules as regular or extern structs, which isn't true anymore with stage2. Thus, a lot of confusion is created and people misuse packed structs accidently or try to solve problems with them where extern struct is the right hammer.

I propose to change the name of packed struct into something that clearly represents what it is. My current best guess would be bitrecord or bitfield, and respectivly rename packed union to bitunion (which is not a good name).

This change is also a good idea as you cannot put arrays or pointers into packed structs

@dee0xeed
Copy link

This change is also a good idea as you cannot put arrays or pointers into packed structs

I would say there is no need at all to hold pointers in packed structs - for me they are just very convenient feature when dealing with various binary network protocols or binary file formats.

@ghost
Copy link

ghost commented Sep 15, 2022

I think bitfield would have been ideal, if it weren't for the analogous bitunion, which is quite terrible. Maybe the same clarification value can be achieved by renaming packed to bitpacked? Thinking further, extern could then become bytepacked.

@leecannon
Copy link
Contributor

leecannon commented Sep 15, 2022

@zzyxyzz extern are not byte packed, you have to add explicit align(1) to non-byte aligned fields to get that behaviour

const HasPadding = extern struct {
    b: bool,
    n: u32,

    comptime {
        std.debug.assert(@sizeOf(HasPadding) == 8);
    }
};

const NoPadding = extern struct {
    b: bool,
    n: u32 align(1),

    comptime {
        std.debug.assert(@sizeOf(NoPadding) == 5);
    }
};

@ghost
Copy link

ghost commented Sep 15, 2022

@leecannon, thanks for the correction.
In that case, bytepacked could become an independent qualifier rather than replacing extern.

@Vexu Vexu added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Sep 15, 2022
@Vexu Vexu added this to the 0.11.0 milestone Sep 15, 2022
@eLeCtrOssSnake
Copy link

tl;dr; I think both names show exact purpose and are fine. I am against changing them.

extern struct - struct for usage in external modules/code/functions thus "extern". packed struct means bitpacked, which means it bitpacks integers. I think both names nail their task 100%. Andre Weissflog said on zig discord that he mistook packed struct with attribute((packed)) in C. But if we follow the zig philosophy then we shouldn't continue to carry this legacy into zig. Because we already have align(1). Packed means a different thing in zig and it's fine.

@ikskuh
Copy link
Contributor Author

ikskuh commented Nov 2, 2022

Andre Weissflog said on zig discord that he mistook packed struct with __attribute__((packed)) in C.

That's exactly what this is about. packed in C means byte packed. packed in Zig means bitpacked. This is an arbitrary choice and both are 100% legal choices. packed in Zig does not convey the information that it is bitpacked, and it's afaik the only language doing this right now. All other languages use bitfield or similar for this kind of construct.

extern in Zig also does not mean "external module/codes/functions", but "well defined memory layout based on the natural alignment of types". Using extern here isn't perfect, but doesn't spark as much confusion as packed. Even people using Zig for longer times confuse this all the time, which means we have a readability defect in the language.

@eLeCtrOssSnake
Copy link

Andre Weissflog said on zig discord that he mistook packed struct with __attribute__((packed)) in C.

That's exactly what this is about. packed in C means byte packed. packed in Zig means bitpacked. This is an arbitrary choice and both are 100% legal choices. packed in Zig does not convey the information that it is bitpacked, and it's afaik the only language doing this right now. All other languages use bitfield or similar for this kind of construct.

extern in Zig also does not mean "external module/codes/functions", but "well defined memory layout based on the natural alignment of types". Using extern here isn't perfect, but doesn't spark as much confusion as packed. Even people using Zig for longer times confuse this all the time, which means we have a readability defect in the language.

I don't understand the difference between extern and packed being a different level of readability defect because extern serves as much of a different purpose in Zig compared to C as packed.

@eLeCtrOssSnake
Copy link

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

@ghost
Copy link

ghost commented Nov 2, 2022

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I would argue that this issue goes deeper than naming and superficial differences between languages. I feel that bitpacking, while certainly a useful feauture, is the wrong default for dense, fixed-layout structures due to their limited composability: you cannot embed arrays or other non-packed structs, and you can have neither field pointers nor pointer fields. Byte-packed structs have none of these problems and should be the default packed layout unless you plan to do actual bit slicing.

However, since the packed struct semantics was changed, Zig no longer has a linear byte-packed struct. Yes, you can use extern with align(1), but besides being a bit of a pain, it is an abuse of a type intended for something else. From the documentation:

An extern struct has in-memory layout guaranteed to match the C ABI for the target. This kind of struct should only be used for compatibility with the C ABI.

extern struct does not actually guarantee any fixed layout, and even if it did, you'd have to know some details about the C ABI to be sure.

The expectation that packed struct should behave like _attribute__((packed)) is not just an accident of history. It actually makes sense. Hence I'd be very much in favor of reintroducing byte-packed struct semantics alongside bitfield, possibly with a syntax like packed(.bit), packed(.byte), packed(.C) as suggested by @tleydxdy in the other thread.

@eLeCtrOssSnake
Copy link

eLeCtrOssSnake commented Nov 2, 2022

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I would argue that this issue goes deeper than naming and superficial differences between languages. I feel that bitpacking, while certainly a useful feauture, is the wrong default for dense, fixed-layout structures due to their limited composability: you cannot embed arrays or other non-packed structs, and you can have neither field pointers nor pointer fields. Byte-packed structs have none of these problems and should be the default packed layout unless you plan to do actual bit slicing.

However, since the packed struct semantics was changed, Zig no longer has a linear byte-packed struct. Yes, you can use extern with align(1), but besides being a bit of a pain, it is an abuse of a type intended for something else. From the documentation:

An extern struct has in-memory layout guaranteed to match the C ABI for the target. This kind of struct should only be used for compatibility with the C ABI.

extern struct does not actually guarantee any fixed layout, and even if it did, you'd have to know some details about the C ABI to be sure.

The expectation that packed struct should behave like _attribute__((packed)) is not just an accident of history. It actually makes sense. Hence I'd be very much in favor of reintroducing byte-packed struct semantics alongside bitfield, possibly with a syntax like packed(.bit), packed(.byte), packed(.C) as suggested by @tleydxdy in the other thread.

Yes, there are things to clarify. extern struct actually won't depend on C ABI if you don't use C types. This is kind of unwritten rule as of now. packed struct makes a compact memory representation and it doesn't guarantee memory layout.
While I still do not agree that packed struct and attribute((packed)) should mean the same thing, I do think we can do better by clarifying which semantics are for what purpose. For example default packed struct is compact representation of a struct by bitpacking, extern struct is for C ABI compatibility and/or guaranteed memory layout.

My proposal is to make clarify that extern can also be used for guarenteed memory layout, packed is NOT attribute((packed)), and for alignning to 1 you should do extern align(1).
I don't see how changing packed or extern keywords for struct would somehow reduce confusion.

edit:
And I think that making "packed(.bit), packed(.byte), packed(.C)" WILL FURTHER more cause confusion because for aligning semantics we already have align()

@praschke
Copy link
Contributor

praschke commented Nov 2, 2022

Right now, a lot of people assume that packed struct follows similar rules as regular or extern structs, which isn't true anymore with stage2. Thus, a lot of confusion is created and people misuse packed structs accidently or try to solve problems with them where extern struct is the right hammer.

I think before changing the name based on peoples' confusion, the documentation for extern struct should perhaps first be changed from

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct or normal struct.

With such a skeletal and foreboding warning, I don't think it is any wonder people think packed struct is what you should use for defining file headers and such.

EDIT: I will admit, I wouldn't be against this proposal if bitunion wasn't such a terrible name. The documentation needs to be fleshed out either way.

@candrewlee14
Copy link
Contributor

candrewlee14 commented Nov 2, 2022

How about just renaming packed to bitfield? Then it’d bebitfield struct and bitfield union, with no bitunion.

@eLeCtrOssSnake
Copy link

How about just renaming packed to bitfield? Then it’d bebitfield struct and bitfield union, with no bitunion.

bitfield and bitunion are horrible names.

@dee0xeed
Copy link

dee0xeed commented Nov 3, 2022

Andre Weissflog said on zig discord that he mistook packed struct with attribute((packed)) in C

me too.

@dee0xeed
Copy link

dee0xeed commented Nov 3, 2022

// Equivalent of C's __attribute__((__packed__))
const S = extern struct {
    a: u64 align(1),
    b: u8 align(1),
};

__attribute__((__packed__)) means there is no spaces between u64 and u8 fields,
so the total size is 9 bytes. It does not tell nothing about how an instance
of this struct is aligned in RAM, right?

extern keyword as a word is neither about packing nor about alignment
and thus is a reason of confusion - 'combine extern and align?..'

btw, hello to middle endian byte order! :)

@nektro
Copy link
Contributor

nektro commented Nov 3, 2022

related: #13009

@jecolon
Copy link

jecolon commented Nov 3, 2022

unpadded struct
bit struct

@InKryption
Copy link
Contributor

How about:

  • bitrecord(uXX) struct
  • bitrecord(uXX) union

@eLeCtrOssSnake
Copy link

Maybe we should just clarify what packing we need when using packed struct:
// My bit packed struct
const MyStruct = packed(.bit) struct {
...
};

// My byte packed struct
const MyStruct = packed(.byte) struct {
...
};

So packed becomes sort of modifier to struct.
Or alternatively make packed accept the number of bits instead of .bit and .byte

@praschke
Copy link
Contributor

i think bit struct and bit(u8) union, etc feels fine enough to me to just replace packed with bit. there's no need to squish it into one word.

@dmbfm
Copy link

dmbfm commented Dec 27, 2022

One thing that really got me confused is that according to the documentation for extern struct:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct.

But this seems to be false. There are use cases of extern structs which are not only for C ABI compability. For instance if I have a color struct:

const Rgb = struct {
   r: u8,
   g: u8,
   b: u8,
};

and I want it to be so an array [_] Rgb be densedly packed in memory (so that I can @memcpy or read directly), then, according to the documentation the regular struct does not give any guarantees as to layout, and extern struct is only for ABI compability. This led me to think that packed struct was the answer in this case; but it was not, since when using packed struct I get @sizeOf(Rgb) = 4 (it adds a one byte padding).

I don't really care about the naming that much, but would be nice if the documentation was more specific in this instance.

@eLeCtrOssSnake
Copy link

One thing that really got me confused is that according to the documentation for extern struct:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct.

But this seems to be false. There are use cases of extern structs which are not only for C ABI compability. For instance if I have a color struct:

const Rgb = struct {
   r: u8,
   g: u8,
   b: u8,
};

and I want it to be so an array [_] Rgb be densedly packed in memory (so that I can @memcpy or read directly), then, according to the documentation the regular struct does not give any guarantees as to layout, and extern struct is only for ABI compability. This led me to think that packed struct was the answer in this case; but it was not, since when using packed struct I get @sizeOf(Rgb) = 4 (it adds a one byte padding).

I don't really care about the naming that much, but would be nice if the documentation was more specific in this instance.

Yes, this was basically my idea. I personally like the naming, just the docs need to be adjusted.

@BogdanTheGeek
Copy link

I also think packed is not representative of what its actually doing.
I don't know @andrewrk 's opinion on what types and attributes should be in zig, but here is an idea from someone who has to do memory mapped devices with 14bit words every other day.

If types are better than attributes:

  • struct - should be "compiler take the wheel" and make it fast or small
  • union - should be "compiler, I promise I won't use both at the same time"
  • enum - is an enum, it has no size or maximum value, its just a list of possibilities
  • bitfield - 1 bit packed memory where the size of the field is the type size. Basically a bitmask shifted by the field index
  • packet - a collection of other types, unions and structs that are all compressed to the smallest size possible.
  • padding - manual padding. Can be any size of bits, tagged or not. uses the standard default assignment syntax, but defaults to undefined, so you don't try to read it. (bad things happen when you try to read reserved registers)

So putting something in a packet makes it "packed". Packets don't care about alignment, if it needs to be aligned to some something, it will have padding, otherwise it will be compact. Arrays with also work just fine as they will be aligned to their size.
The only issues are see with this are:

  • no guaranteed size for enums, how do you pack an enum? In C, you would set the last value at the MAX_U(type) and pack it. You could also say that enums in a union assume the largest known size type, but then why use the enum...
  • What is an array of bool in a packet? To me, it would be the same as a bitfield that you can slice. Why? Why not?

The other side of the coin would favour attributes:

  • ordered - guarantees members order (basically packed stage2)
  • condensed, compact or folded - does what people would expect packed to do, smallest possible object, order is not guaranteed
  • extern - uses the C ABI

So, to get the equivalent of a packet, one would use compact ordered struct.
A compact union is the same as a packed union
A compact struct is a bitfield

The problems I can see with this solution are:

  • compact enums these would no longer be zero width? I am not quite sure how enums are packed currently...
  • They are not types, so I don't like them

I personally would prefer first class types for this. I think packet is the natural evolution of C bit unions and packed structs and padding would be a god send for embedded.
With the risk of this being considered a manifesto, here is a real life example for this, it should be very easy to read even if you are not an electrical engineer:

// https://www.mouser.co.uk/datasheet/2/268/MCP7940N_Battery_Backed_I2C_RTCC_with_SRAM_2000501-2937048.pdf
// MCP7940N RTC with battery backup

const I2C_Command = bitfield {
    address: u7,
    write: bool,
}

const I2C_Packet = packet {
    command: I2C_Command,
    data: u8,
}

const MCP7940N_AlarmMask = enum u3 {
    SECONDS = 0,
    MINUTES = 1,
    HOURS = 2,
    DAY = 3,
    DATE = 4,
    RESERVED1 = 5,
    RESERVED2 = 6,
    ALL = 7,
}

// Could be a bitfield so padding would be implicit, but this is more explicit
const MCP7940N = packet {
    sec: bitfield {
        value: u7,
        status: const bool,
    },
    min: bitfield {
        value: u7,
        const padding = 0,
    },
    hour: bitfield {
        union {
            bitfield {
                value12: u4,
                pm: bool,
            },
            value24: u5,
            24hr: bool,
        },
        const padding = 0,
    },
    day: bitfield {
        value: u3,
        vbat: const bool,
        pwrfail: const bool,
        oscrun: const bool,
        [2]const padding,
    },
    date: bitfield {
        value: u6,
        [2]const padding,
    },
    month: bitfield {
        value: u5,
        leap: bool,
        [2]const padding,
    },
    year: bitfield {
        value: u8,
    },
};

I could have defined the default register values too, but this is just a quick demo.
This would also address #13009

@nmichaels
Copy link
Contributor

nmichaels commented Jul 7, 2023

The whole argument is that somebody confuses packed with C packed and now suddenly we have to change it. But we're not C. We're a different language.

I think the criticism that C's packed and Zig's packed mean very different things is entirely valid. zig.org's front page suggests adding "a Zig compilation unit to C/C++ projects." It makes sense that we might acknowledge that features in C have names.

Some things in C (like calling private things static, and having things be public by default) were, in Zig's opinion, mistakes. That's fair. I agree. However, the word "packed" for structs that don't have any padding seems pretty sensible to me. I'm not sure we should consume the valuable friction we're allotted by having it mean something entirely different.

@zxubian
Copy link

zxubian commented Jul 8, 2023

I think the core issue is not just that "packed" in Zig means something different to C.
It's that

  1. having a struct with no padding between fields (a tightly-packed struct, if you will) is a common requirement for a low-level language
  2. the current way of achieving this very common use-case in Zig (extern struct & align(1)) is unintuitive, has poor readability and discoverability both for people familiar with C, and those not familiar.

Personally, when I want to control the memory layout of a struct, I reach for words like "layout", "padding", "packing" etc. I don't immediately think of "extern".

Conversely, I think the code snippet by @BogdanTheGeek above was very easy to read.

@dmbfm
Copy link

dmbfm commented Jul 8, 2023

  1. the current way of achieving this very common use-case in Zig (extern struct & align(1)) is unintuitive, has poor readability and discoverability both for people familiar with C, and those not familiar.

And the documentation makes things even more confusing by saying that:

This kind of struct should only be used for compatibility with the C ABI. Every other use case should be solved with packed struct or normal struct.

Which simply isn't true, since you use extern struct to get a struct without padding, which has more uses than just C interop.

@nmichaels
Copy link
Contributor

And the documentation makes things even more confusing by saying that:

I think we're all agreed that the documentation is misleading and confusing, but maybe it shouldn't be. Maybe extern struct should really only be for C interoperability, in which case I think we need a new thing (or to re-repurpose packed) for the common use case of a struct whose job is to map to some sort of data structure that's represented outside of just in-memory.

Whether the name for that is unpadded, packet (while I like @BogdanTheGeek's suggestion I don't love the word packet), compact, packed, or some as-yet unsuggested but brilliant word, I think it's important to get agreement that this is a language feature we want to have, distinct from extern struct with align(1). Or maybe it's just sugar.

Personally, I think packed is a good name for it, and the thing that's currently called packed, if it needs to exist, should get a different one. Then we can leave the extern struct documentation alone and write a whole bunch of other documentation.

@kj4tmp
Copy link
Contributor

kj4tmp commented Jun 23, 2024

I also hit this confusion when trying to understand the expected behavior of translate-c when encountering a #pragma pack

#20405

@Cloudef
Copy link
Contributor

Cloudef commented Jun 28, 2024

I agree with this, there should be a clear distinction between bitpacked and bytepacked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests