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

Implement Eq, Clone and Hash for MemoryData and Evaluator #387

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Jun 30, 2018

In order to implement infinite loop detection while executing MIR, both the implementer of Machine (Evaluator) and its associated type (MemoryData), must implement Eq, Clone and Hash. This PR adds the required trait implementations.

It's possible that the Hash implementations need to be improved; only the env_vars field of Evaluator and the thread_local field of MemoryData are actually being hashed. Omitting fields from a Hash implementation is not incorrect, but could lead to collisions if the ignored fields are changing constantly. Perhaps I should instead derive Hash on a few more fields related to MIR validation?

In order to implement infinite loop detection while executing MIR, both
the implementor of `Machine` (`Evaluator`) and its associated type
(`MemoryData`), must implement `Eq` and `Hash`. This PR adds the
required trait implementations.

It's possible that the `Hash` implementations need to be improved; only
the `env_vars` field of `Evaluator` and the `thread_local` field of
`MemoryData` are actually being hashed. Omitting fields from a `Hash`
implementation is not incorrect, but could lead to collisions if the
ignored fields are changing constantly. Perhaps I should instead derive
`Hash` on a few more fields related to MIR validation?
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Jun 30, 2018

This depends on some of my commits in rustc being merged. I'm not sure how to express this requirement.

@RalfJung
Copy link
Member

RalfJung commented Jul 3, 2018

I wouldn't bother with the validation fields, but what about the memory content?

Also, this seems extremely costly -- are you essentially storing snapshots to see if one reoccurs? Why should that not eat up all RAM immediately?
(EDIT: Anyway that's to be discussed on the rustc side and I see you had some pretty awesome ideas there. :D )

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2018

but what about the memory content?

memory content is currently cared about on the rustc side, this is just for extra things that rustc doesn't know

@RalfJung
Copy link
Member

Seems the rustc side of this got merged, so we should get this landed soon-ish?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 11, 2018

Yea, but we need a new nightly before Travis will accept the PR

@RalfJung
Copy link
Member

How is it possible that #395 is still green on Travis?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2018

We didn't get a new nightly

@RalfJung
Copy link
Member

Thanks :)

@RalfJung RalfJung merged commit ff64b42 into rust-lang:master Jul 13, 2018
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.

3 participants