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

Guarantee the layout of structs with a single non-zero-sized field #164

Merged
merged 9 commits into from
Aug 27, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 8, 2019

This PR guarantees the layout of structs with a single non-zero-sized field to be the same layout as that of that field if the alignment requirements of all other fields is 1.

Closes #34 .

@gnzlbg gnzlbg mentioned this pull request Jul 9, 2019
@Lokathor
Copy link
Contributor

Can we make this guarantee stronger? It seems like as long as all ZST values have an alignment less than or equal to the Non-ZST value that layout should be unaffected.

eg: *mut T and *mut T, PhantomData<&mut T> should fairly probably be definitely the same.

@hanna-kruppe
Copy link

PhantomData<T> has alignment 1 regardless of T so it's already covered.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 24, 2019

Can we make this guarantee stronger?

We could. This was not discussed in the issue, but I suppose that we can make it stronger as a follow up.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 24, 2019

@rkruppe @RalfJung what do you think about strengthening the guarantee to:

The default layout of structs with a single non-zero-sized field is the same as
the layout of that field if the alignment requirement of all other fields is less than or equal to
the alignment requirement of the non-zero-sized field.

?

@hanna-kruppe
Copy link

I don't think we should innovate here (has this wording ever popped up anywhere before?), especially as @Lokathor's motivation for proposing it (PhantomData) doesn't need this stronger guarantee after all. The only additional things this strengthening covers are empty arrays and user-declared ZSTs with an explicit repr(align(N)) neither of which seems important to me. It also deviates from the precedent set by repr(transparent) -- in a sense we're doing this to downgrade repr(transparent) from a very important opt-in to a guardrail and arguably shouldn't go further.

@RalfJung
Copy link
Member

Unblocking as the 1-ZST PR landed. Or was this blocked on something else?

@RalfJung RalfJung removed the blocked label Jul 26, 2019
@RalfJung RalfJung added the A-layout Topic: Related to data structure layout (`#[repr]`) label Aug 14, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

So I've updated this to use the "1-ZST" terminology and noticed two things:

struct S([0; u8], [0; u8], [0; u16]);

to be the same as that of [0; u 16] by, instead of guaranteeing this for structs with one non-zero-sized field, guaranteeing those for structs that have one field that is not a 1-ZST.

I am not sure whether that is worth doing, and tend to agree with @rkruppe that the simple guarantee is enough. Just food for thought. What isn't clear to me is how we could generalize such a guarantee in the spirit of what @Lokathor proposed here, but without mentioning that one type is a non-zero-sized type.

@RalfJung
Copy link
Member

structs that have one field that is not a 1-ZST.

I think this is what we should do. It is most consistent. The current definition leaves a weird "gap".

@RalfJung
Copy link
Member

the text is a bit harder to read (maybe there is a better way to use the "1-ZST" terminology?) - I also noticed this in #161 .

I think this might be due to the mismatch mentioned/"gap" in my previous post. Or do you think this is hard to read?

"The default layout of structs where only a single field is not a 1-ZST, is the same as the layout of said field".

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 16, 2019

Done.

@RalfJung
Copy link
Member

RalfJung commented Aug 16, 2019

In fact, I personally would just go for the stronger statement that for struct layout, 1-ZST are ignored. I don't see any reason to guarantee/propose some instances of that without guaranteeing/proposing the general, simpler to explain and more useful, principle.

Both the current content of this PR and "structs consisting only of 1-ZST are 1-ZST" then basically just fall out of that principle, and the additional orthogonal statemenets

  • structs with no fields (and no repr annotation) are 1-ZST
  • structs with 1 field (and no repr annotation) have the same layout as that field

struct S2(Zst2, Zst1);
```

the layout of `S1` is the same as that of `i32` and the layout of `S2` as that of `Zst2`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is saying more than just "1-ZST fields are ignored".

You are also assuming here that single-field structs have layout identical to their only field. That is a separate statement, and should IMO get a separate section.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This comment is still open.)

@hanna-kruppe
Copy link

hanna-kruppe commented Aug 16, 2019

"1-ZST fields are ignored for layout" seems to go beyond what got consensus back when struct layout was discussed. For example, I think it would mean that all instantiations of this type (i.e., regardless of the unit) have the same layout:

struct Vec2<Unit> {
    x: f32,
    y: f32,
    _unit: PhantomData<Unit>
}

but as was discussed extensively, there is some desire to keep open the possibilities of randomized and profile-driven type layout decision, and none of the very small restrictions on that (single-field structs, single-variant enums, etc.) that were proposed without anyone objecting cover this.

Don't get me wrong, I am personally also very much in favor of layout ignoring 1-ZSTs universally (and many other things besides). But I don't think we should just "sneak in" something which was explicitly discussed and didn't get consensus.

Edit: on second thought I'm not so sure anymore, since arguably Vec2<A> and Vec2<B> are still different (types, substs) tuples even if the PhantomData field is ignored? That seems painfully subtle and lawyering-prone, though.

@RalfJung
Copy link
Member

For example, I think it would mean that all instantiations of this type (i.e., regardless of the unit) have the same layout:

I don't see how it would say that. Those are still all separate instances of a generic type. The 1-ZST is ignored, but the type parameter is not.

@hanna-kruppe
Copy link

I realized the same thing after posting and just finished editing it in 😅

@RalfJung
Copy link
Member

RalfJung commented Aug 16, 2019

@rkruppe in other words, we don't guarantee that the following two have the same layout:

struct Vec2_1 {
    x: f32,
    y: f32,
}
struct Vec2_2 {
    x: f32,
    y: f32,
}

Likewise, we don't guarantee that all instance of these have the same layout:

struct Vec2Plus<T> {
    x: f32,
    y: f32,
    plus: *mut T,
}

No ZST are involved in any of this. Why would the ZST in your example make any difference?

The fact that Vec2<A> and Vec2<B> don't have to have the same layout is an instance of the same principle. We have nominal typing, and every different choice of parameters is a different "name".

EDIT: Ah, you part-way agree. Well see this post for why we need that lawyering anyway, and the ZST question is entirely orthogonal. ;)

@RalfJung
Copy link
Member

Opened a PR for my remaining comments: gnzlbg#2

rearrange a bit and be more explicit about how our rules interact
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rkruppe what do you think?

@hanna-kruppe
Copy link

I'm perpetually confused by what any given person means by "layout", didn't we at some point agree to stop using that term to prevent confusion? Assuming that in this diff "layout" = size, align, field offsets and padding, possibly niches but definitely not call ABI, the content seems fine. But I'm unsure whether, editorially speaking, we should already be avoiding "layout" in new paragraphs or if that's gonna be a separate clean-up PR that goes over all the documents.

@RalfJung
Copy link
Member

That would be #153. I think the plan (like in the other PR) was to land this PR with the vague wording so that we have some examples to work with when finalizing the terminology. It's much easier to do that when you have some examples.

@gnzlbg gnzlbg merged commit 308c474 into rust-lang:master Aug 27, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 27, 2019

What we mean by layout here will be precised in #153 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Topic: Related to data structure layout (`#[repr]`)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layout of single-field structs
4 participants