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

Make slices (optionally) packed #2201

Closed
Sahnvour opened this issue Apr 6, 2019 · 9 comments
Closed

Make slices (optionally) packed #2201

Sahnvour opened this issue Apr 6, 2019 · 9 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@Sahnvour
Copy link
Contributor

Sahnvour commented Apr 6, 2019

They currently are not, forbidding this usecase:

test "" {
    const Foo = packed struct {
        bar: []u32,
    };
}
error: non-packed, non-extern struct '[]u32' not allowed in packed struct; no guaranteed in-memory representation
    bar: []u32,
    ^

As long as no debug safety is embedded within them, I see no reason that they couldn't be packed.

@Sahnvour Sahnvour added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Apr 6, 2019
@Sahnvour Sahnvour added this to the 0.5.0 milestone Apr 6, 2019
@Sahnvour
Copy link
Contributor Author

Sahnvour commented Jun 8, 2019

So in my PR there is an error because one function is fed with a pointer and a length as separate parameters, coming from a slice. The function expects default alignment, and the slice fields are accessed as align(1) because of ir_analyze_container_field_ptr as @LemonBoy pointed out.

It's easy to make it work, just add align(1) to the function parameters' types, however I'm unsure about the piece of code making everything in a packed struct aligned on 1 byte boundary.

Is this a temporary state of things, or a decision needed/set in stone ? I feel like it could be avoided in cases where the fields actually do not need to be align(1), as is the case for slices (the container type is aligned to 8, and so can .ptr and .len).

Also note that in this particular case, it's probably only an issue when interfacing with C code, because idiomatic zig code would use slices directly and not decompose them.

@andrewrk I'm interested on your thoughts about this.

@daurnimator
Copy link
Contributor

Related to #2652?

@Sahnvour
Copy link
Contributor Author

Unless the len field can be made to use a smaller int than usize, I don't think so. And that would likely require to specify it in the slice type, so not really practical. What do you have in mind ?

@daurnimator
Copy link
Contributor

daurnimator commented Jun 13, 2019

Unless the len field can be made to use a smaller int than usize

That wasn't what I was thinking of, but actually it could: the len is never going to be more than usize / max(@sizeOf(T), @alignOf(T)). You can also (on at least x86_64 linux) take off another 1 bit for userspace vs kernel space.

What do you have in mind ?

I meant the same thing as in the packed pointer proposal: taking off the bits unused due to pointer alignment.

@andrewrk
Copy link
Member

andrewrk commented Jul 8, 2019

As long as no debug safety is embedded within them, I see no reason that they couldn't be packed.

That's just it though - if they don't have debug safety, then slices would not be able to participate in, for example, #2414.

I think there may be a better way to solve the use case outlined here.

After #1512 I think we can relax the restriction about what types can go in packed structs. A packed struct would have well-defined memory layout if and only if all its fields did. So your above example would work, but Foo would have a field in it with undefined memory layout. I think we want this ability for packed structs anyway.

I'd like to solve #1512 as a prerequisite for this proposal, so I'm going to postpone this proposal for next release cycle.

@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Jul 8, 2019
@DutchGhost
Copy link

Fun fact,

test "" {
    const Foo = packed union {
        bar: []u32,
    };
}

is currently allowed

@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Jan 7, 2020
@andrewrk
Copy link
Member

It's not planned to have optionally packed slices, however it is planned that packed structs will be allowed to contain types which do not have well defined memory layouts.

@daurnimator
Copy link
Contributor

however it is planned that packed structs will be allowed to contain types which do not have well defined memory layouts.

Is there an open (and accepted) issue for that?

@tadeokondrak
Copy link
Contributor

@daurnimator #3133

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

Successfully merging a pull request may close this issue.

5 participants