-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix ICE when using Box<T, A> with pointer sized A #94043
Conversation
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
@@ -330,7 +330,7 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> { | |||
ty::Ref(..) | ty::RawPtr(_) => { | |||
return self.field(cx, index).llvm_type(cx); | |||
} | |||
ty::Adt(def, _) if def.is_box() => { | |||
ty::Adt(def, substs) if def.is_box() && cx.layout_of(substs.type_at(1)).is_zst() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a comment here stating that we only handle wide pointer boxes as pointers, not thin pointer boxes with a scalar allocator. That case is handled in the general logic below
@bors r+ rollup |
📌 Commit d0b508e has been approved by |
…askrgr Rollup of 9 pull requests Successful merges: - rust-lang#93337 (Update tracking issue numbers for inline assembly sub-features) - rust-lang#93758 (Improve comments about type folding/visiting.) - rust-lang#93780 (Generate list instead of div items in sidebar) - rust-lang#93976 (Add MAIN_SEPARATOR_STR) - rust-lang#94011 (Even more let_else adoptions) - rust-lang#94041 (Add a `try_collect()` helper method to `Iterator`) - rust-lang#94043 (Fix ICE when using Box<T, A> with pointer sized A) - rust-lang#94082 (Remove CFG_PLATFORM) - rust-lang#94085 (Clippy: Don't lint `needless_borrow` in method receiver positions) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fix ICE when using Box<T, A> with large A A sequel to rust-lang#94043 that fixes rust-lang#81270 and rust-lang#92054 (duplicate).
ty::Adt(def, _) if def.is_box() => { | ||
// only wide pointer boxes are handled as pointers | ||
// thin pointer boxes with scalar allocators are handled by the general logic below | ||
ty::Adt(def, substs) if def.is_box() && cx.layout_of(substs.type_at(1)).is_zst() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @antoyo I think cg_gcc needs the same fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like applying the same fix cause another issue.
Do you think there is the same issue for the LLVM codegen?
cc @drmeepster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the problem is that in the mini_core
test, Box doesn't have the allocator type parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fixed it in cg_clif in bjorn3/rustc_codegen_cranelift@401b034. Feel free to copy the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, that doesn't solve the issue. I made changes more similar to liballoc itself, but got another ICE:
thread 'rustc' panicked at 'assertion failed: sig.c_variadic || extra_args.is_empty()', compiler/rustc_middle/src/ty/layout.rs:3027:13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you change Box
to include an extra field (eg ()
) and change box_free
to add an extra argument of the same type? They need to be in sync. I got the exact same error when I hadn't changed box_free
yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
What I meant is that I still get the error fixed by this PR with your changes:
thread 'rustc' panicked at 'index out of bounds: the len is 1 but the index is 1', /rustc/4b043faba34ccc053a4d0110634c323f6c03765e/compiler/rustc_middle/src/ty/subst.rs:364:43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean.
My own fixes miss the change you made to box_free
.
Thanks. It works now!
…eta, r=michaelwoerister Only emit pointer-like metadata for `Box<T, A>` when `A` is ZST Basically copy the change in rust-lang#94043, but for debuginfo. r? `@michaelwoerister` Fixes rust-lang#94725
…eta, r=michaelwoerister Only emit pointer-like metadata for `Box<T, A>` when `A` is ZST Basically copy the change in rust-lang#94043, but for debuginfo. r? ``@michaelwoerister`` Fixes rust-lang#94725
Fix ICE when using Box<T, A>, again Sequel to rust-lang#94043, fixes rust-lang#94835.
Remove dereferencing of Box from codegen Through rust-lang#94043, rust-lang#94414, rust-lang#94873, and rust-lang#95328, I've been fixing issues caused by Box being treated like a pointer when it is not a pointer. However, these PRs just introduced special cases for Box. This PR removes those special cases and instead transforms a deref of Box into a deref of the pointer it contains. Hopefully, this is the end of the Box<T, A> ICEs.
Fixes #78459
Note that using
Box<T, A>
with a more than pointer sizedA
or using a pointer sizedA
with a Box of a DST will produce a different ICE (#92054) which is not fixed by this PR.