Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

dwrensha
Copy link
Contributor

@dwrensha dwrensha commented Jan 6, 2018

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

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@oli-obk
Copy link
Contributor

oli-obk commented Jan 8, 2018

I don't think this'll work for ((),(),((), &i32)) or similarly nested things.

@michaelwoerister
Copy link
Member

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Jan 8, 2018

IMO this should "just" do untupling by doing what arg0 = tuple.0; arg1 = tuple.1; ... would do.
Which means using project_field or w/e and avoid all of this broken makeshift logic.

@carols10cents carols10cents added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 9, 2018
@kennytm
Copy link
Member

kennytm commented Jan 17, 2018

Hi @dwrensha, just to check if you're still working on this PR 😊

Could you apply @eddyb's suggestion in #47239 (comment)?

@dwrensha
Copy link
Contributor Author

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.

@eddyb
Copy link
Member

eddyb commented Jan 20, 2018

My point was that the entire match args[1].value { can become a single loop using place_field.

@kennytm
Copy link
Member

kennytm commented Jan 31, 2018

Hi @dwrensha, checking again. Would you like to apply the suggested change in #47239 (comment), i.e. to refactor “the entire match into a single loop using place_field”? Or should we just close it since the intention was “mainly to bring attention to the problem”?

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.

@pietroalbini
Copy link
Member

@dwrensha and @oli-obk, ping from triage!

@oli-obk
Copy link
Contributor

oli-obk commented Feb 5, 2018

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

@pietroalbini
Copy link
Member

Ok, closing this. @dwrensha, thank you for this PR anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants