More layout sanity checks

[RalfJung]

r? @eddyb

[eddyb]

_{(this is most of a comment first written when I started a review on the original state of the PR but didn't submit it right away)}

You could deal with the large file by putting the checks in ty/layout_checks.rs or ty/layout/checks.rs (the latter would mean mod checks; in layout.rs and would be able to do use super::*; to improve everything including private items - I have mixed feelings but it's practical I think).

_{(thoughts unactionable in this PR follow below)}

Honestly we should consider to what crate to move the entire layout module, since it's mostly just providing queries, and it feels awkward where it is.

I also wish it was rustc_middle::ty::abi or rustc_middle::abi (could even go into mir but I doubt that makes as much sense) for consistency with rustc_target::abi but that's another mess.

[RalfJung]

I wish we used the term 'abi' less, not more, given how ambiguous it is.^^

[eddyb]

Could this have 1zst/1_zst or align1zst/align1_zst in the name instead of zst?

[RalfJung]

It doesn't actually produce 1-ZST though, I had to change it to produce all ZST. Will adjust the comment.

[RalfJung]

FWIW this function seems useful to have elsewhere as well? Not sure where to put it though.

[eddyb]

Hmm, so you know how tools like pointee_info_at dig into a type to find primitive leaves?

It might be easier to make an abstraction like this, if only for the layout checks (note: "cases" below refers to enum Variants::Multiple and union fields):

// HACK(eddyb) used below to disambiguate between the two uses of `Size`,
// but we should probably have this as a completely separate type, with:
// `Size`: measured in bytes and/or bits; Add+Mul ops (Sub not needed)
// `Offset`: measured in bytes (no "bit offsets"); Add+Sub ops (Mul not needed)
type Offset = Size;

// HACK(eddyb) necessary method sadly missing from `std::ops::Range`.
impl<T> Range<T> {
    fn includes(&self, other: Range<T>) -> bool {
        self.start <= other.start && other.end <= self.end
    }
}

struct ScalarLeaf {
    /// Where in the parent layout this leaf is.
    ///
    /// Must be the right size, i.e. `extent.end - extent.start == primitive`.
    //
    // FIXME(eddyb) does the above condition make `extent.end` redundant?
    // (move to a `fn extent(&self, cx: ...) -> Range<Offset>` helper method?)
    extent: Range<Offset>,

    // FIXME(eddyb) instead of ignoring validity ranges, should they be merged
    // across e.g. `enum` variants?
    primitive: Primitive,
}

struct ScalarLeafWithinError {
    /// No leaves found in `search_extent`.
    OnlyPadding,

    /// A leaf intersects `search_extent` without being contained in it,
    /// i.e. the leaf is "straddling" `search_extent`'s start or end.
    PartialIntersection(ScalarLeaf),

    /// Two leaves found within `search_extent` that differ in their `extent`,
    /// `primitive`, or both.
    Mismatch(ScalarLeaf, ScalarLeaf),
}

/// Find the unique `Scalar` (but ignoring validity ranges, so... `Primitive`¹?)
/// leaf field wholly contained within `search_extent` in `layout`.
/// 
/// An `Ok(leaf)` result indicates a leaf field that is:
/// * "wholly contained within": `search_extent.includes(leaf.extent)`
/// * "unique": `layout` always contains (across all `enum`/`union` cases):
///   * `Scalar(leaf.primitive)` exactly filling `leaf.extent`
///     * some `enum`/`union` cases may contain padding at `leaf.extent` instead,
///       but all other `enum`/`union` cases must agree on the same `leaf`
///     * `Scalar` validity might vary, but the `Primitive` must match exactly
///   * only padding, in `search_extent` "around" `leaf.extent`,
///     i.e. neither of these extents contain any leaves of their own:
///     * `search_extent.start..leaf.extent.start` (before `leaf`)
///     * `leaf.extent.end..search_extent.end` (after `leaf`)
/// 
/// ¹ I wish `Primitive` was called `Scalar` but what would we call `Scalar`?
///   (maybe `Primitive` should be renamed to `ScalarType`/`ScalarKind`?)
fn scalar_leaf_within(
    layout: &TyAndLayout<'tcx>,
    search_extent: Range<Offset>,
) -> Result<ScalarLeaf, ScalarLeafWithinError> {
    fn try_for_each_leaf_intersecting<E>(
        base_offset: Offset,
        layout: &TyAndLayout<'tcx>,
        filter_extent: Range<Offset>,
        each: &mut impl FnMut(ScalarLeaf) -> Result<(), E>
    ) -> Result<(), E> {
        for (i, field_offset) in layout.fields.enumerate() {
            if field_offset < filter_extent.end {
                let field = layout.field(i);
                if filter_extent.start < field_offset + field.size {
                    try_for_each_leaf_intersecting(
                        base_offset + field_offset,
                        field,
                        filter_extent,
                        each,
                    )?;
                }
            }
        }
        for v in 0..layout.variants.len() {
            try_for_each_leaf_intersecting(base_offset, layout.for_variant(v), filter_extent, each)?;
        }

        if layout.fields.is_empty() && !layout.is_zst() {
            match layout.abi {
                Abi::Scalar(scalar) => each(ScalarLeaf {
                    extent: base_offset..base_offset+scalar.size(),
                    primitive: scalar.primitive,
                })?,

                // FIXME(eddyb) can we guarantee all non-ZST leaves are `Scalar`?
                _ => unreachable!(),
            }
        }

        Ok(())
    }

    let mut found = None;
    try_for_each_leaf_intersecting(
        Offset::ZERO,
        layout,
        search_extent,
        |candidate| match found {
            _ if !search_extent.includes(&candidate.extent) => {
                Err(ScalarLeafError::PartialIntersection(candidate))
            }
            Some(previous) if candidate != previous => {
                Err(ScalarLeafError::Mismatch(previous, candidate))
            }
            _ => {
                found = Some(candidate);
                Ok(())
            }
        }
    )?;
    Ok(found.ok_or(ScalarLeafWithinError::OnlyPadding))
}

With that abstraction in hand, we can define the conditions for:

• Abi::Scalar(scalar):

// HACK(eddyb) needed to deal with `Offset` vs `Size` type differences.
let layout_extent = Offset::ZERO..Offset::ZERO+layout.size;

assert_eq!(
    scalar_leaf_within(layout, layout_extent),
    Ok(ScalarLeaf {
        extent: layout_extent,
        primitive: scalar.primitive,
    }),
);

• Abi::ScalarPair(a, b):

// HACK(eddyb) needed to deal with `Offset` vs `Size` type differences.
let layout_extent = Offset::ZERO..Offset::ZERO+layout.size;
let a_extent = Offset::ZERO..Offset::ZERO+a.size;
// FIXME(eddyb) there should be a nicer way to make `start..start+len` ranges.
let b_extent = a_extent.end.align_to(b.align)..;
let b_extent = b_extent.start..b_extent.start+b.size;

assert_eq!(
    scalar_leaf_within(layout, layout_extent.start..b_extent.start),
    Ok(ScalarLeaf {
        extent: a_extent,
        primitive: a.primitive,
    }),
);
assert_eq!(
    scalar_leaf_within(layout, b_extent.start..layout_extent.end),
    Ok(ScalarLeaf {
        extent: b_extent,
        primitive: b.primitive,
    }),
);

This should cover any field hierarchy, but here are some extra thoughts:

• I wasted spent a lot of time on designing scalar_leaf_within's interface,
  but try_for_each_leaf_intersecting might be better to use directly?
  • its filter_extent could be removed if we outright just want to iterate all
    leaves and for e.g. Abi::ScalarPair(a, b) we check whether every found leaf
    is either matching a or b (in extent and primitive)
• this isn't trying to tackle Scalar validity ranges, but I don't think that's
  possible without making scalar_leaf_within recursive (which I avoided above
  to make it simpler, as I'm effectively flattening all the leaves together),
  and having logic for merging across enum variants and union fields
  • even then, you'd still have to handle e.g. NonZero* types and niches
• we should probably have a separate Offset from Size no matter what
• the whole "extent" terminology was an attempt at minimizing ambiguity/confusion
  over ideas like "a subslice of a layout" (as in, the subset of a layout's
  fields that occupy bytes exclusively in a subrange of 0..layout.size),
  but I'm not sure it's optimal at all, ideas welcome

[eddyb]

r=me if you don't want to take up getting The Fun Layout Search pseudocode working (would still be good to have an issue tracking how good these checks are etc.)

[RalfJung]

I opened an issue at #100231, also linking specifically to your proposed checks.

@bors r=eddyb rollup=iffy

[bors]

📌 Commit 27013d2 has been approved by eddyb

It is now in the queue for this repository.

[bors]

⌛ Testing commit 27013d2 with merge 5a9c3a2...

[bors]

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing 5a9c3a2 to master...

[rust-timer]

Finished benchmarking commit (5a9c3a2): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.5%	2.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.8%	2.8%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²