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: don't allow unused bits in packed unions #19754

Open
jacobly0 opened this issue Apr 23, 2024 · 4 comments
Open

Proposal: don't allow unused bits in packed unions #19754

jacobly0 opened this issue Apr 23, 2024 · 4 comments
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@jacobly0
Copy link
Member

jacobly0 commented Apr 23, 2024

Currently it is valid to do:

const U = packed union {
    x: u8,
    y: u16,
};

However, there is not plainly one possible way of mapping this representation to bits, a feature of other packed types that seems desirable. For example, enum (u5) { ... } plainly represents 5 bits in an obvious manner and is allowed in packed contexts, but ?u8 has two reasonable ways of mapping to 9 bits and is not allowed in packed contexts.

My proposal for resolving this ambiguity is to require all fields of a packed union to have the same @bitSizeOf as a required backing integer type. The above type is still representable as:

const U = packed union(u16) {
    x: packed struct(u16) {
        used: u8,
        unused: u8 = undefined,
    },
    y: u16,
};

But this type now has the benefit of being explicit about the order of the unused bits and therefore only has one reasonable bitwise representation. Also, depending on how #19634 is resolved, this explicitness could be very important as having an undefined field in a packed container may become much less useful in the future, and it may be necessary to initialize to 0 instead for many use cases. To that end, this proposal encourages that explicit thought is put into how the padding bits should be defined.

Related:

@jacobly0 jacobly0 added proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Apr 23, 2024
@jacobly0 jacobly0 added this to the 0.14.0 milestone Apr 23, 2024
@Rexicon226
Copy link
Contributor

Rexicon226 commented Apr 23, 2024

A small change I propose is to instead of requiring them to have the same @bitSizeOf, which could lead to some weird behaviour and error messages like in cases such as:

const U = packed union {
    a: u7,
    b: u8, 
    c: u8,
    d: u9,
}

What would the error here be? Either

  1. it would resolve to being u8 as the majority, and u7 and u9 are wrong
  2. it'll coerce to the largest size which might not be desired

Instead I think it makes more sense to unify the current semantics, and have packed struct(u8) and packed union(u8), and the "same @bitSizeOf" must match the provided backing int.

@jacobly0
Copy link
Member Author

jacobly0 commented Apr 23, 2024

and have packed struct(u8) and packed union(u8)

Actually, thinking about it some more, this should definitely be a requirement for many different reasons. The most obvious is that an 8-bit packed union should be allowed in callconv(.C) function parameters, but the C abi of u8 and i8 differ, so the user needs to specify which one to use!

@jacobly0 jacobly0 added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Apr 23, 2024
@andrewrk andrewrk modified the milestones: 0.14.0, 0.13.0 Apr 24, 2024
@andrewrk andrewrk added the accepted This proposal is planned. label Apr 24, 2024
@expikr
Copy link
Contributor

expikr commented Apr 24, 2024

Can multiple unused fields use _ as their field name provided that they have a default initialization value?

@mochalins
Copy link

#19395 seems relevant here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. frontend Tokenization, parsing, AstGen, Sema, and Liveness. 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

5 participants