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.

Sign up for GitHub

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

Jump to bottom

Fix ICE when using Box<T, A> with pointer sized A #94043

Merged
merged 2 commits into from
Feb 18, 2022
Merged

Fix ICE when using Box<T, A> with pointer sized A #94043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae32f43
fix assumption that ScalarPair Box is always a fat pointer
beepster4096 Feb 16, 2022
d0b508e
add comment explaining the check
beepster4096 Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_llvm/src/type_of.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,9 @@ 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() => {
// 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() => {
Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Contributor

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!

let ptr_ty = cx.tcx.mk_mut_ptr(self.ty.boxed_ty());
return cx.layout_of(ptr_ty).scalar_pair_element_llvm_type(cx, index, immediate);
}
Expand Down
6 changes: 6 additions & 0 deletions src/test/ui/box/issue-78459-ice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// check-pass
#![feature(allocator_api)]

fn main() {
Box::new_in((), &std::alloc::Global);
}