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

[WIP] Remove #![feature(label_break_value)] (fix #731) #872

Closed
wants to merge 5 commits into from

Conversation

bungcip
Copy link
Contributor

@bungcip bungcip commented Mar 23, 2023

remove #![feature(label_break_value)] from generated rust source file.
rust-toolchain is updated to nightly-2022-10-25

@bungcip
Copy link
Contributor Author

bungcip commented Mar 23, 2023

unit test fail in tests/filecheck/aggregate1.rs:
original aggregate1.rs.analysis.txt :

Aggregate literal label: Label { origin: None, origin_params: [('a, Origin(4)), ('h0, Origin(5))], perm: (empty) }#S[]

after upgrading rust-toolchain:

Aggregate literal label: Label { origin: None, origin_params: [('a, Origin(4)), ('h0, Origin(5))], perm: (empty) }#S<'_>[]

checking c2rust-analyze/src/labeled_ty.rs :

impl<'tcx, L: fmt::Debug> fmt::Debug for LabeledTyS<'tcx, L> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{:?}#{:?}{:?}", self.label, self.ty, self.args)
    }
}

self.ty is rustc_middle::ty::Ty. maybe the output format changed -_-;
need more investigation...

@bungcip bungcip force-pushed the upgrade-toolchain branch from 7376167 to 37a641a Compare March 25, 2023 04:33
@rinon
Copy link
Contributor

rinon commented Mar 27, 2023

Thanks for working on this! There's two separate things going on here, rolling the toolchain forward and removing the then unnecessary feature. The commit message/title should at least reflect this, as rolling the toolchain version is more important than the feature removal. The feature removal is small enough of a change that I think it's ok to combine with the toolchain bump.

This looks mostly fine once expected test output is updated. However the cast changes of Misc to PtrToPtr are a bit unexpected. I assume the compiler removed Misc, but that cast was for pointer to usize casts, which doesn't seem like a PtrToPtr cast.

@spernsteiner @aneksteind is bumping the toolchain forward ok with what you're in the middle of right now?

@spernsteiner
Copy link
Collaborator

c2rust-analyze changes look good to me.

is bumping the toolchain forward ok with what you're in the middle of right now?

Fine with me, as long as everything builds and tests still pass.

However the cast changes of Misc to PtrToPtr are a bit unexpected. I assume the compiler removed Misc, but that cast was for pointer to usize casts, which doesn't seem like a PtrToPtr cast.

They've broken up Misc into a bunch of more specific variants: https://doc.rust-lang.org/stable/nightly-rustc/rustc_middle/mir/enum.CastKind.html
Pointer to usize falls under the new CastKind::PointerExposeAddress

@kkysen
Copy link
Contributor

kkysen commented Mar 27, 2023

Hi @bungcip, thanks for working on this! I wanted to let you know, though, that we've already previously worked on updating the toolchain to a more recent version in #811, but have since closed that PR as updating the toolchain wasn't a priority for us yet (in the long term, it is helpful, though). I just wanted to let you know so you don't accidentally redo work already done, and so you can look at that PR for help in updating the toolchain (it contains some other necessary fixes, including a bug fix, along with reasoning). The clippy fixes in that PR need to be removed as they were since removed in clippy in a Rust patch release, but otherwise it should still be applicable besides needing a rebase.

Specifically for PtrToPtr, that is the correct cast in that situation. It's a fat to thin ptr cast that's done immediately before the thin ptr to usize cast.

@bungcip
Copy link
Contributor Author

bungcip commented Mar 28, 2023

hi @kkysen, thanks for the review. maybe I will close the PR. I actually want to working on issue #621 or #769 for improving c2rust output . To familiarize myself with c2rust code, I am trying to fix #731 because its simple enough, unfortunately to fix the issue, rust-toolchain must be updated and going to fixing rustc_private changes rabid hole is too soul crushing for me 😅

@bungcip bungcip closed this Mar 28, 2023
@bungcip bungcip deleted the upgrade-toolchain branch April 2, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants