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

distinguish "no data" from "heterogeneous" in ABI #57645

Merged
merged 1 commit into from
Jan 26, 2019

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 15, 2019

Ignore zero-sized types when computing whether something is a homogeneous aggregate, except be careful of VLA.

cc #56877

r? @arielb1
cc @eddyb

@nikomatsakis
Copy link
Contributor Author

PS, @glandium, you may want to try this build to see if it fixes your problem =)

@nikomatsakis
Copy link
Contributor Author

One interesting test where the right behavior is not obvious:

#[repr(C)]
pub struct MiddleFieldPhantom {
    pub a: f32,
    pub b: f32,
    pub c: [u32; 0],
    pub d: PhantomData<u8>,
}

/// In the case where the VLA is in the middle, but the only thing
/// that comes next is phantomdata, we still consider that to be
/// `has_vla: false`. Not clear if that is correct but also not clear
/// that it is wrong.
#[rustc_layout(abi)]
pub type Test4 = MiddleFieldPhantom;
//~^ ERROR abi: Aggregate { sized: true, has_vla: false }

@rust-highfive

This comment has been minimized.

@nikomatsakis
Copy link
Contributor Author

Tests just needed to be blessed. Hopefully ok now.

@nikomatsakis
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Jan 16, 2019

⌛ Trying commit 6beebd0d109004f208b4241229ecedf12a752971 with merge 896270e14094a65293b52bbbf9b447c557dda848...

@nikomatsakis
Copy link
Contributor Author

@glandium when the try build above completes, we should produce an aarch64 artifact that you can evaluate. It can be installed using the rustup-toolchain-install-master utility.

@bors
Copy link
Contributor

bors commented Jan 16, 2019

☔ The latest upstream changes (presumably #57321) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 16, 2019
@bors
Copy link
Contributor

bors commented Jan 16, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@glandium
Copy link
Contributor

@glandium when the try build above completes, we should produce an aarch64 artifact that you can evaluate. It can be installed using the rustup-toolchain-install-master utility.

I don't know what commit to give rust-toolchain-install-master. I tried 6beebd0d109004f208b4241229ecedf12a752971 and 896270e14094a65293b52bbbf9b447c557dda848, which is the last couple (commit, merge) linked by bors here, but it all ends up in 404s.

@nikomatsakis
Copy link
Contributor Author

@glandium I think 896270e is correct; not sure why it didn't work, maybe @pietroalbini or @Mark-Simulacrum have a suggestion (they're the ones who advised me on how to do it)

@pietroalbini
Copy link
Member

This command works for me:

rustup-toolchain-install-master --host aarch64-unknown-linux-gnu 896270e14094a65293b52bbbf9b447c557dda848

@glandium
Copy link
Contributor

So it looks like bors only built aarch64-unknown-linux-gnu host rustc? I can't test those, because we don't build Firefox on those. For aarch64-pc-windows-msvc, we build on x86_64-pc-windows-msvc.

@pietroalbini
Copy link
Member

We can't do try builds for windows at the moment though.

@@ -550,7 +572,7 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {
let abi = if count != 0 && ty.conservative_is_privately_uninhabited(tcx) {
Abi::Uninhabited
} else {
Abi::Aggregate { sized: true }
Abi::Aggregate { sized: true, has_vla: (count == 0) }
Copy link
Member

Choose a reason for hiding this comment

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

Is a check for repr.c() not warranted here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, never mind, its an array, which is repr(C) by definition.

Copy link
Member

Choose a reason for hiding this comment

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

See #56877 (comment) about VLA-ness of 0-sized arrays.

src/librustc/ty/layout.rs Outdated Show resolved Hide resolved
@@ -706,6 +738,13 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> {
};
align = align.max(field_align);

// Track whether any field had the "VLA flag.
if !any_variant_has_vla {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason other than a gut feeling for short-circuiting the code inside? To me it seems like a likely no-op performance wise.

if any_variant_has_vla && def.repr.c() {
if let Abi::Aggregate { has_vla, .. } = &mut abi {
*has_vla = true;
}
Copy link
Member

@nagisa nagisa Jan 18, 2019

Choose a reason for hiding this comment

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

Similarly a style nit, this could be written as just

if let Abi::Aggregate { has_vla, .. } = &mut abi {
    *has_vla |= any_variant_has_vla && def.repr.c();
}

/// a homogeneous aggregate.
///
/// (Note that this field is always false for things that are
/// not `#[repr(C)]`.)
Copy link
Member

Choose a reason for hiding this comment

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

Note that this field is always false for things that are not #[repr(C)]

Seems like a difficult requirement to uphold in the future without something asserting this somewhere. I feel like the code downstream should be able to handle this field regardless of the overall ABI (i.e. interpret it semantically).


If an aggregate has a VLA within itself, it itself becomes variable-length, in which case it would make sense to just name this field as is_variable_length...

Which then brings up a question: does it make sense for a variable-length aggregate to also be sized? Well, at least the #[repr(C)] struct Foo { last: [T; 0] } would be Aggregate { sized: true, variable_length: true }, but then what really is it – a variable length aggregate or a zero-sized aggregate? Hmm...

To be fair, I’m not sure what ABI interactions arise here – the currently implemented behaviour may well be the correct one...

@nagisa
Copy link
Member

nagisa commented Jan 18, 2019

I feel like this could still use a codegen test for aarch regression, but sadly our codegen testing infrastructure does not currently allow testing assembly...

@glandium
Copy link
Contributor

We can't do try builds for windows at the moment though.

I'm told this affects Fennec for aarch64 Android as well, and we build those on Linux. Can you do a try x86_64-pc-linux-gnu build (with aarch64 std)?

@arielb1
Copy link
Contributor

arielb1 commented Jan 20, 2019

@nikomatsakis

I'll look at this when I'll have the time - don't know when will that be.

@nikomatsakis
Copy link
Contributor Author

I found this comment by @parched somewhat persuasive -- it basically argues that we don't really have a way to represent "VLA" in Rust today (it's almost a [T] type, except that a pointer to it should not be a fat pointer), and that -- in any case -- it is basically incorrect to pass a "variable-length struct" by value, so we should err on the side of assuming that [T; 0] actually means a zero-length array and not a variable-length array. I think this implies that we could simplify this PR by basically removing the has_vla logic and just filtering out zero-sized things during the homogeneous aggregate test.

@glandium
Copy link
Contributor

Can you do a try x86_64-pc-linux-gnu build (with aarch64 std)?

@pnkfelix
Copy link
Member

@nikomatsakis wrote:

I've updated the PR to only exclude repr(rust) structs. I believe this is strictly better than status quo. It is also somewhat dissatisfying to me, but I'd really like to land something here and it seems like there is consensus on going forward with this minimal set.

can you update the PR description to reflect this change, so that the eventual bors commit will have a commit message that reflects reality?

@nikomatsakis
Copy link
Contributor Author

@eddyb I believe this now implements the change you wanted in the way that you wanted it =) if you approve, I'll squash the commits.

@nikomatsakis nikomatsakis changed the title abi aggregates ignore zero-sized data in abi aggregates Jan 24, 2019
@nikomatsakis
Copy link
Contributor Author

Squashed and rebased.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Jan 24, 2019

r=me with description updated

Also, add a testing infrastructure and tests that lets us dump layout.
@nikomatsakis nikomatsakis changed the title ignore zero-sized data in abi aggregates distinguish "no data" from "heterogeneous" in ABI Jan 25, 2019
@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb p=1

Giving high priority because this is blocking some folks at Mozilla

@bors
Copy link
Contributor

bors commented Jan 25, 2019

📌 Commit 8e4c57f has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 25, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 25, 2019
…ates, r=eddyb

distinguish "no data" from "heterogeneous" in ABI

Ignore zero-sized types when computing whether something is a homogeneous aggregate, except be careful of VLA.

cc rust-lang#56877

r? @arielb1
cc @eddyb
bors added a commit that referenced this pull request Jan 25, 2019
Rollup of 5 pull requests

Successful merges:

 - #56233 (Miri and miri-related code contains repetitions of `(n << amt) >> amt`)
 - #57645 (distinguish "no data" from "heterogeneous" in ABI)
 - #57734 (Fix evaluating trivial drop glue in constants)
 - #57886 (Add suggestion for moving type declaration before associated type bindings in generic arguments.)
 - #57890 (Fix wording in diagnostics page)

Failed merges:

r? @ghost
@bors bors merged commit 8e4c57f into rust-lang:master Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants