-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix miri argument unpacking issue #47239
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
I don't think this'll work for |
r? @eddyb |
IMO this should "just" do untupling by doing what |
Hi @dwrensha, just to check if you're still working on this PR 😊 Could you apply @eddyb's suggestion in #47239 (comment)? |
I've updated Seer with a somewhat simplified version of this, including the testcase @oli-obk was concerned about: dwrensha/seer@f2962ea. Unfortunately, the https://github.com/solson/miri testsuite is not currently runnable at rustc master (see rust-lang/miri#361), so it is difficult to verify whether changes like this are correct. I'll defer to @oli-obk and @eddyb on what to do with this pull request. My intent was mainly to bring attention to the problem. |
My point was that the entire |
Hi @dwrensha, checking again. Would you like to apply the suggested change in #47239 (comment), i.e. to refactor “the entire Also cc @oli-obk, as OP mentioned in #47239 (comment), it seems difficult to move this PR forward without miri testing re-enabled, which in turn is blocked by #46882. |
Let's close this until I get the miri submodule working again. Until then it's not testable and additionally this PR is not the preferred way to solve this |
Ok, closing this. @dwrensha, thank you for this PR anyway! |
This should fix rust-lang/miri#360. I haven't actually tested it.
I believe this fix is correct because a similar fix works for Seer: dwrensha/seer@a071e1c
cc @oli-obk