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

Wrap const_eval_stack_frame_limit in a LockCell #56085

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Aaron1011
Contributor

Aaron1011 commented Nov 19, 2018

This allows Miri to set const_eval_stack_frame_limit as needed,
while still keeping Session thread-safe

Wrap const_eval_stack_frame_limit in a OneThread<Cell>
This allows Miri to set const_eval_stack_frame_limit as needed,
while still keeping Session thread-safe
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 19, 2018

r? @varkor

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

@Aaron1011

This comment has been minimized.

Contributor

Aaron1011 commented Nov 19, 2018

r? @Zoxc

@rust-highfive rust-highfive assigned Zoxc and unassigned varkor Nov 19, 2018

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Nov 19, 2018

This doesn't look correct since Miri could be used on multiple threads.

@Aaron1011

This comment has been minimized.

Contributor

Aaron1011 commented Nov 20, 2018

Miri currently doesn't support multiple threads. I don't believe anyone is currently working on adding multithreading support, so I think this change should be fine for the forseeable future.

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Nov 20, 2018

Miri can't emulate multiple threads, but you can have Miri executions on multiple threads in rustc currently.

Use LockCell instead of OneThread<Cell>
Miri can be run from multiple rustc threads

@Aaron1011 Aaron1011 changed the title from Wrap const_eval_stack_frame_limit in a OneThread<Cell> to Wrap const_eval_stack_frame_limit in a LockCell Nov 20, 2018

@Aaron1011

This comment has been minimized.

Contributor

Aaron1011 commented Nov 20, 2018

@Zoxc: I've switched const_eval_stack_frame_limit to use a LockCell

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Nov 20, 2018

I'd like to know how you plan on changing const_eval_stack_frame_limit in Miri in order to check that it's actually thread-safe. I don't think you could change const_eval_stack_frame_limit using LockCell in the rustc driver in a race-free way.

@Aaron1011

This comment has been minimized.

Contributor

Aaron1011 commented Nov 20, 2018

@Zoxc: I'll be changing it here, before Miri starting running the target function.

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Nov 21, 2018

That doesn't seem correct either, assuming that the Miri driver will use rustc's const_eval for consistency. I think the correct move would be to add a stack_limit field to EvalContext and have the Miri driver populate that differently from rustc's const_eval. cc @oli-obk

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 21, 2018

cc @RalfJung

We should probably remove this field entirely and make the engine depend on the recursion limit that is also used for macros and such. This way users can control the recursion depth directly from the source

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 21, 2018

@oli-obk Actually I think miri-the-tool wants no limit at all here, TBH.

For CTFE, the "normal" recursion limit seems fine. What is its default value?

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 21, 2018

What is its default value?

update_limit(sess, krate, &sess.recursion_limit, "recursion_limit",
"recursion limit", 64);

Actually I think miri-the-tool wants no limit at all here, TBH.

That seems like an easy change to make by making the check a Machine-method

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 21, 2018

That seems like an easy change to make by making the check a Machine-method

Actually, wouldn't the stack_push hook added by #56094 suffice for that?

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Nov 21, 2018

Yes that seems like the correct place to check that

@TimNN

This comment has been minimized.

Contributor

TimNN commented Dec 4, 2018

Ping from triage @Aaron1011: It looks like some changes have been requested to your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment