distinguish "no data" from "heterogeneous" in ABI

[nikomatsakis]

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

cc #56877

r? @arielb1
cc @eddyb

[nikomatsakis]

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

[nikomatsakis]

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 }

[nikomatsakis]

Tests just needed to be blessed. Hopefully ok now.

[nikomatsakis]

@bors try

[bors]

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

[nikomatsakis]

@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]

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

[bors]

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

[glandium]

@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]

@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)

[emilyalbini]

This command works for me:

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

[glandium]

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.

[emilyalbini]

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

[nagisa]

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

[nagisa]

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

[nagisa]

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

[nagisa]

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.

[nagisa]

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();
}

[nagisa]

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]

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]

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]

@nikomatsakis

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

[nikomatsakis]

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]

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

[pnkfelix]

@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]

@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]

Squashed and rebased.

r? @eddyb

[eddyb]

r=me with description updated

[nikomatsakis]

@bors r=eddyb p=1

Giving high priority because this is blocking some folks at Mozilla

[bors]

📌 Commit 8e4c57f has been approved by eddyb