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

skip known panics lint for impossible items #123169

Closed
wants to merge 1 commit into from

Conversation

lukas-code
Copy link
Member

fixes #123134

For items with impossible predicates like [u8]: Sized it's possible to have locals that are "Sized", but cannot be const-propped in any meaningful way. To avoid this issue, we can just skip the known panics lint for items that have impossible predicates.

@rustbot
Copy link
Collaborator

rustbot commented Mar 28, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Mar 28, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@oli-obk
Copy link
Contributor

oli-obk commented Mar 28, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 28, 2024
@bors
Copy link
Contributor

bors commented Mar 28, 2024

⌛ Trying commit 7b46e02 with merge 76192f0...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
skip known panics lint for impossible items

fixes rust-lang#123134

For items with impossible predicates like `[u8]: Sized` it's possible to have locals that are "Sized", but cannot be const-propped in any meaningful way. To avoid this issue, we can just skip the known panics lint for items that have impossible predicates.
/// Checks whether an item is impossible to reference.
#[instrument(level = "debug", skip(tcx), ret)]
fn is_impossible_item<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> bool {
let param_env = tcx.param_env_reveal_all_normalized(def_id);
Copy link
Member

@compiler-errors compiler-errors Mar 28, 2024

Choose a reason for hiding this comment

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

This is kind of strange. Can you just use elaborate(tcx.predicates_of().instantiate_identity).filter(|clause| !clause.has_param()).collect()?

It shouldn't matter to eagerly fold opaques -- we're already processing impossible_predicates with a reveal_all param-env.

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work, because it would filter out local predicates like T::Assoc: Sized, even if they normalize to global predicates like [u8]: Sized, which is exactly the case here:

impl<T> Adapter for T
{
    type A = ApiS;
    fn open() -> OpenDevice<Self::A>
    where
        <Self::A as Api>::Device: Sized,
      // ^^^^ This `Self` is an alias for the type parameter `T`
      // The `predicates_of` has the "local" predicate `<T::A as Api>::Device: Sized`,
      // but the `param_env` (with or without reveal all) has the "global" predicate `[u8]: Sized`
    {
        unreachable!()
    }
}

So, we need to normalize the predicates_of before filtering out !clause.has_param(). And the the normalized + elaborated predicates_of is exactly the param_env. I'm using param_env_reveal_all_normalized instead of param_env here as an optimization, exactly because impossible_predicates will normalize in the empty reveal-all env anyway.

Copy link
Member

@compiler-errors compiler-errors Mar 28, 2024

Choose a reason for hiding this comment

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

Okay. Still feels a bit hacky that you're using param_env.caller_bounds() as a substitute for, e.g. normalize_erasing_regions-ing the elaborated caller bounds or something. This should at least be documented.

I'm using param_env_reveal_all_normalized instead of param_env here as an optimization, exactly because impossible_predicates will normalize in the empty reveal-all env anyway.

I don't think this is a particularly compelling optimization, especially because the param_env query has definitely been fetched for all items we care about, and the likelihood of having an opaque in a where clause anyways is very small. I'd prefer if we write the code more explicitly unless it's proven to be hot, which I doubt is the case for this code, since it's gonna be only running once per item.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still feels a bit hacky that you're using param_env.caller_bounds() as a substitute for, e.g. normalize_erasing_regions-ing the elaborated caller bounds or something.

Do you mean normalize_erasing_regions-ing the elaborated predicates_of?

Copy link
Member

Choose a reason for hiding this comment

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

ya typo

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so normalizing the predicates individually doesn't work, because sometimes we have two predicates like <T as Foo>::Assoc: Bar and <T as Foo>::Assoc == SomeType so that one predicate can only be normalized if the other predicate is in the param env. That's why the implementation of param_env does that whole normalizing the param env in itself thing. This also means that my implementation here will stop working once we stop eagerly normalizing the param env.

Also one UI test will overflow if we normalize the predicates individually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright I found a way that seems to work: We can just normalize the predicates_of individually inside the param_env of the item.

@bors
Copy link
Contributor

bors commented Mar 28, 2024

☀️ Try build successful - checks-actions
Build commit: 76192f0 (76192f036189e2ee162445ee163df829c3c2d3c4)

@rust-timer

This comment has been minimized.

@compiler-errors
Copy link
Member

Unrelated to the above discussion, but I'm somewhat worried that this is a spot-fix for a more general class of issues where we're calling layout_of on types in param-envs that have trivially impossible preds throughout the later stages of the compiler.

I wonder if it's better to just fix this in the layout code, e.g. only asserting these validity requirements if we're totally monomorphic, like if param_env == ty::ParamEnv::reveal_all(), and otherwise returning a LayoutError instead.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (76192f0): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.6% [0.2%, 1.1%] 13
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
7.3% [5.7%, 10.4%] 3
Improvements ✅
(primary)
-1.5% [-1.9%, -1.0%] 14
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.5% [-1.9%, -1.0%] 14

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.5% [0.4%, 2.6%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [-0.5%, 2.6%] 3

Binary size

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

Bootstrap: 669.029s -> 668.895s (-0.02%)
Artifact size: 315.66 MiB -> 315.66 MiB (-0.00%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 28, 2024
@lukas-code
Copy link
Member Author

Alright, looks like this will definitely still ICE for other MIR passes, so this PR is clearly not the solution and I'll go ahead and close it as such. Removing the delayed bug from the layout computation ( #123169 (comment) ) is probably a better approach.

code for GVN ICE
//@ build-pass

trait Api: Sized {
    type Device: ?Sized;
}

struct OpenDevice<A: Api>
where
    A::Device: Sized,
{
    device: A::Device,
    queue: (),
}

trait Adapter {
    type A: Api;

    fn open() -> usize
    where
        <Self::A as Api>::Device: Sized;
}

struct ApiS;

impl Api for ApiS {
    type Device = [u8];
}

impl<T> Adapter for T
{
    type A = ApiS;

    fn open() -> usize
    where
        <Self::A as Api>::Device: Sized,
    {
        1 + std::mem::size_of::<OpenDevice<Self::A>>()
    }
}

fn main() {}

@lukas-code lukas-code closed this Mar 28, 2024
@lukas-code lukas-code deleted the known-panics-ice branch March 28, 2024 22:35
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
…ler-errors

layout computation: gracefully handle unsized types in unexpected locations

This PR reworks the layout computation to eagerly return an error when encountering an unsized field where a sized field was expected, rather than delaying a bug and attempting to recover a layout. This is required, because with trivially false where clauses like `[T]: Sized`, any field can possible be an unsized type, without causing a compile error.

Since this PR removes the `delayed_bug` method from the `LayoutCalculator` trait, it essentially becomes the same as the `HasDataLayout` trait, so I've also refactored the `LayoutCalculator` to be a simple wrapper struct around a type that implements `HasDataLayout`.

The majority of the diff is whitespace changes, so viewing with whitespace ignored is advised.

implements rust-lang#123169 (comment)

r? `@compiler-errors` or compiler

fixes rust-lang#123134
fixes rust-lang#124182
fixes rust-lang#126939
fixes rust-lang#127737
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 17, 2024
layout computation: gracefully handle unsized types in unexpected locations

This PR reworks the layout computation to eagerly return an error when encountering an unsized field where a sized field was expected, rather than delaying a bug and attempting to recover a layout. This is required, because with trivially false where clauses like `[T]: Sized`, any field can possible be an unsized type, without causing a compile error.

Since this PR removes the `delayed_bug` method from the `LayoutCalculator` trait, it essentially becomes the same as the `HasDataLayout` trait, so I've also refactored the `LayoutCalculator` to be a simple wrapper struct around a type that implements `HasDataLayout`.

The majority of the diff is whitespace changes, so viewing with whitespace ignored is advised.

implements rust-lang/rust#123169 (comment)

r? `@compiler-errors` or compiler

fixes rust-lang/rust#123134
fixes rust-lang/rust#124182
fixes rust-lang/rust#126939
fixes rust-lang/rust#127737
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Sep 25, 2024
layout computation: gracefully handle unsized types in unexpected locations

This PR reworks the layout computation to eagerly return an error when encountering an unsized field where a sized field was expected, rather than delaying a bug and attempting to recover a layout. This is required, because with trivially false where clauses like `[T]: Sized`, any field can possible be an unsized type, without causing a compile error.

Since this PR removes the `delayed_bug` method from the `LayoutCalculator` trait, it essentially becomes the same as the `HasDataLayout` trait, so I've also refactored the `LayoutCalculator` to be a simple wrapper struct around a type that implements `HasDataLayout`.

The majority of the diff is whitespace changes, so viewing with whitespace ignored is advised.

implements rust-lang/rust#123169 (comment)

r? `@compiler-errors` or compiler

fixes rust-lang/rust#123134
fixes rust-lang/rust#124182
fixes rust-lang/rust#126939
fixes rust-lang/rust#127737
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

univariant: field #2 comes after unsized field
7 participants