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

Support casting boxes to dyn* #102641

Merged
merged 3 commits into from
Oct 13, 2022
Merged

Support casting boxes to dyn* #102641

merged 3 commits into from
Oct 13, 2022

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
Contributor

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
Contributor

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