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

ui/deriving/issue-89188-gat-hrtb.rs test fails with debug assertions enabled #89639

Closed
asquared31415 opened this issue Oct 7, 2021 · 9 comments · Fixed by #89823
Closed

ui/deriving/issue-89188-gat-hrtb.rs test fails with debug assertions enabled #89639

asquared31415 opened this issue Oct 7, 2021 · 9 comments · Fixed by #89823
Labels
A-GATs Area: Generic associated types (GATs) A-traits Area: Trait system C-bug Category: This is a bug. F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@asquared31415
Copy link
Contributor

The ui/deriving/issue-89188-gat-hrtb.rs test fails due to an arithmetic overflow when debug assertions are enabled.

The offending code likely intends to use the explicit wrapping math methods, as the test does pass with the overflow.

The test was implemented in #89341

Backtrace

thread 'rustc' panicked at 'attempt to subtract with overflow', compiler/rustc_trait_selection/src/traits/project.rs:561:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.57.0-dev running on x86_64-unknown-linux-gnu

note: compiler flags: -Z threads=1 -Z ui-testing -Z deduplicate-diagnostics=no -Z emit-future-incompat-report -C codegen-units=1 -C prefer-dynamic -C rpath -C debuginfo=0

query stack during panic:
#0 [specialization_graph_of] building specialization graph of trait `core::clone::Clone`
#1 [coherent_trait] coherence checking all impls of trait `core::clone::Clone`
end of query stack

@asquared31415 asquared31415 added the C-bug Category: This is a bug. label Oct 7, 2021
@camelid camelid added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 7, 2021
@camelid
Copy link
Member

camelid commented Oct 7, 2021

The overflowing code is

let index =
self.universe_indices.len() - debruijn.as_usize() + self.current_index.as_usize() - 1;

To me, it actually seems like the overflow might be a bug. @jackh726 it looks like you added this code. Is it incorrect for it to be overflowing?

@camelid camelid added I-prioritize Issue: Indicates that prioritization has been requested for this issue. A-traits Area: Trait system F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs and removed A-testsuite Area: The testsuite used to check the correctness of rustc labels Oct 7, 2021
@nbdd0121
Copy link
Contributor

nbdd0121 commented Oct 7, 2021

This is introduced in #86993 with guards like if debruijn.as_usize() + 1 > self.current_index.as_usize() + self.universe_indices.len() => bug!() near call-site. So I suppose just swapping the order should be sufficient?

@jackh726
Copy link
Member

jackh726 commented Oct 8, 2021

Overflowing here is a bug. Given that we haven't seen an index out of bound panics on like 562, I would guess that the actual logic here is correct (so, we underflow, then overflow). I do want to prove to myself that again (and put it in a comment).

@the8472
Copy link
Member

the8472 commented Oct 8, 2021

If it is really intended then wrapping_sub should be used.

@tlyu
Copy link
Contributor

tlyu commented Oct 8, 2021

Oh, I think I ran into this as well and thought it was due to a local change I had made violating an invariant.

@camsteffen
Copy link
Contributor

Duplicate of #89280?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 14, 2021

Duplicate of #89280?

no, that issue is in diagnostics, while this one is in trait resolution. I'll try to get to the diagnostics issue next week

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 14, 2021
Switch order of terms to prevent overflow

Fixes rust-lang#89639

r? `@pnkfelix`
@apiraino apiraino added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 14, 2021
@apiraino
Copy link
Contributor

@rustbot label -I-prioritize

@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 14, 2021
@camsteffen
Copy link
Contributor

Duplicate of #89280?

no, that issue is in diagnostics, while this one is in trait resolution. I'll try to get to the diagnostics issue next week

@oli-obk Ah my bad, thanks. I think I've been running in to both issues and conflating them.

@bors bors closed this as completed in 29081f9 Oct 14, 2021
@fmease fmease added the A-GATs Area: Generic associated types (GATs) label Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-GATs Area: Generic associated types (GATs) A-traits Area: Trait system C-bug Category: This is a bug. F-generic_associated_types `#![feature(generic_associated_types)]` a.k.a. GATs T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.