Skip to content

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Oct 4, 2022

Boxes have a pointer type at codegen time which LLVM does not allow to be transparently converted to an integer. Work around this by inserting a ptrtoint instruction if the argument is a pointer.

r? @compiler-errors

Fixes #102427

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 4, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2022
@@ -285,6 +286,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bug!("Only valid to do a DynStar cast into a DynStar type")
};
let vtable = get_vtable(bx.cx(), source.ty(self.mir, bx.tcx()), trait_ref);
let data = match operand.layout.pointee_info_at(bx.cx(), Size::ZERO) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like there ought to be a better way to check if an operand is a pointer.

Also, this will likely be unnecessary once we have the conversion traits.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah pointee_info is definitely the wrong thing to check here. That type is for optimization hints only.

You have the full type layout here, you can just check layout.abi().

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2022
@eholk
Copy link
Contributor Author

eholk commented Oct 5, 2022

Huh, I'm having a hard time reproducing that failure on my machine. The tests are passing for me.

@eholk
Copy link
Contributor Author

eholk commented Oct 5, 2022

Ah, I was able to run the tests in docker on my machine and now I'm getting the same failure. On to debugging!

@eholk
Copy link
Contributor Author

eholk commented Oct 7, 2022

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2022
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 8, 2022

📌 Commit 0c47fdf has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 8, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Oct 8, 2022

Actually wait @bors r-

@eholk sorry but do you mind leaving a comment here with like a FIXME(dyn-star): that summarizes how this is not exactly the ideal way of doing the check? I want some documentation for both why we're doing it, and some signifier that it needs fixing eventually.

then r=me

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 8, 2022
@eholk
Copy link
Contributor Author

eholk commented Oct 12, 2022

Sure, I added a fixme comment. Feel free to r- if you think it needs more detail.

@bors r=@compiler-errors

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

📌 Commit 8b2c3eb has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2022
@compiler-errors
Copy link
Member

LGTM

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 12, 2022
Support casting boxes to dyn*

Boxes have a pointer type at codegen time which LLVM does not allow to be transparently converted to an integer. Work around this by inserting a `ptrtoint` instruction if the argument is a pointer.

r? `@compiler-errors`

Fixes rust-lang#102427
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 13, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#102641 (Support casting boxes to dyn*)
 - rust-lang#102836 (rustc_target: Fix json target specs using LLD linker flavors in link args)
 - rust-lang#102949 (should-skip-this: add missing backslash)
 - rust-lang#102967 (Add test for issue 102964)
 - rust-lang#102971 (tidy: error if a lang feature is already present)
 - rust-lang#102974 (Fix small word dupe typos)
 - rust-lang#102980 (rustdoc: merge separate `.item-info` CSS)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6755c2a into rust-lang:master Oct 13, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 13, 2022
@crlf0710 crlf0710 added the F-dyn_star `#![feature(dyn_star)]` label Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-dyn_star `#![feature(dyn_star)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Boxes can not be cast to dyn*
8 participants