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 niches part of layout and expand call ABI #153

Closed
wants to merge 16 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jun 26, 2019

Closes #122

cc @eddyb


(Once the other PRs are merged I'll update the links here).

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 26, 2019

So I've added some clarifications to the ABI and the examples.

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!

@RalfJung
Copy link
Member

@gnzlbg this PR has conflicts now. Can you resolve that, and fix the nit? Then I am going to merge. This has sat long enough with an "approved" seal and nobody complaining.

@eddyb
Copy link
Member

eddyb commented Jul 18, 2019

Kind of related, we can do the same in the compiler too: rust-lang/rust#62692
(due to initialization concerns, we only can ever use at most once niche)

@gnzlbg gnzlbg force-pushed the layout2 branch 2 times, most recently from 85ecb46 to 9546c7a Compare July 23, 2019 14:18
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 23, 2019

Done.

@RalfJung
Copy link
Member

RalfJung commented Jul 23, 2019

Hm okay I am having second thoughts about merging this without hearing from anyone else in the WG. After all this (re)defines a key term.

@rust-lang/wg-unsafe-code-guidelines please share your thoughts. :)

Also, do we need to update any of the other existing text that talks about "layout"? I am thinking in particular of statements of the form "A and B have the same layout".

@RalfJung
Copy link
Member

Saying incorrect things until the update has been carried out seems bad, though.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 23, 2019

So I went through all uses of "layout" and updated everything except for struct and enum documents.

I don't think anything "incorrect" in these two documents, but they mix the term "layout" with "memory layout" quite freely, and they also use the term C-compatible layout, but AFAICT C has no notion of niches, so I'm unsure what to do there.

@RalfJung
Copy link
Member

Thanks! That looks great.

"C-compatible layout" might be worth defining here? But leaving that for later is also okay.

However, I would like to see an ack of some other WG member before merging.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 23, 2019

"C-compatible layout" might be worth defining here?

I think we need a different term for "size+alignment" (e.g. "memory layout" ?), "size+alignment+call AB" (e.g. "abi" ?), and what Rust uses for layout optimizations (size+alignment+abi+niche, e.g., just "layout").

Otherwise being explicit about each term gets repetitive and error prone, and it is hard to refactor as we iterate on "layout"'s definition.

@RalfJung
Copy link
Member

"size+alignment" (e.g. "memory layout" ?),

I like "shape", but I am not sure what exactly that should include.

@ehsanmok
Copy link
Member

ehsanmok commented Jul 23, 2019

How about

  1. size + alignment -> memory layout
  2. memory layout + call abi -> call layout
  3. call layout + niches -> type layout ?

@Lokathor
Copy link
Contributor

I'd maybe put call layout at the end of the list. There's many data transforms you might care to do that worry about size/align/niche but don't care about call

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.

The conclusion in the meeting was that we should avoid the term "layout" in the reference, and instead define in the glossary which components a layout can have, and then always spell that out explicitly, probably with an abbreviation like "they have the same SAN-layout" [size, alignment, niche].

@RalfJung RalfJung mentioned this pull request Aug 6, 2019
4 tasks
@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

FWIW this isn't stale. The PRs #159, #161, and #164 all use "layout" in the same way that the glossary does now.

I prefer to wait for some version of those to be merged before replacing "layout" with something more specific everywhere, since the diff of doing that might help us tune things a bit.

@RalfJung
Copy link
Member

Also see #122 (comment), where we are going back to the drawing board and considering calling these sub-aspects of "layout" "ABIs". So there would be a bunch of ABIs (call ABI, memory ABI, maybe something like "nesting ABI" that includes a niche and means things are the same even when nested in other types?"), and all of that together would be called "layout". Or so.

@RalfJung RalfJung added the S-blocked Status: Blocked on progress outside of T-opsem's control label Aug 15, 2019
```rust,ignore
enum Abi {
Uninhabited,
Scalar(Scalar),
Copy link
Member

Choose a reason for hiding this comment

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

ScalarPair is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's intentional. ScalarPair is an implementation detail useful for some optimizations that we don't want to guarantee it exists - right @eddyb ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's just an aggregate that we pass around as two scalars only in Rust ABIs and within functions.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

Or so.

Yes. Just to confirm, the temporary plan until we have a better solution is to continue to call this "Layout", but writing down for each case what exactly do we guarantee (SAN-layout), right ?

@RalfJung
Copy link
Member

That was the plan before "X ABI" got proposed and gained some traction. Not sure if it still is the plan.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 15, 2019

I think it would be at least helpful to know what do we precisely guarantee where for coming up with the "X" in the "X ABI" plan.

@RalfJung
Copy link
Member

@gnzlbg given that this will need some rewording and rebasing etc. anyway, are you fine with going ahead with #191 while this PR is still open?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Aug 29, 2019

Sure @Lokathor go ahead, this will need many changes anyways.

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2020

There was no activity here for quite a while so I am going to close this PR; looks like more discussion in #122 is needed.

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]`) S-blocked Status: Blocked on progress outside of T-opsem's control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should niches/ABI be part of the layout of a type?
6 participants