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

Should be able to change stack limit #643

Closed
orium opened this issue Feb 23, 2019 · 7 comments · Fixed by rust-lang/rust#66726
Closed

Should be able to change stack limit #643

orium opened this issue Feb 23, 2019 · 7 comments · Fixed by rust-lang/rust#66726
Labels
A-interpreter Area: affects the core interpreter C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@orium
Copy link
Member

orium commented Feb 23, 2019

It is currently not possible to change the stack limit. It would be great to get that functionality back!

Original report

Maybe I'm doing something wrong, but I'm unable to set the stack limit for miri.

If I call cargo miri test --help it says:

The config flag `miri` is automatically defined for convenience. You can use
it to configure the resource limits

    #![cfg_attr(miri, memory_size = 42)]

available resource limits are `memory_size`, `step_limit`, `stack_limit`

After adding

#![cfg_attr(miri, stack_limit = 100000)]

to my lib.rs I get "stack limit reached error" at the same depth.

@RalfJung
Copy link
Member

@oli-obk did that get lost in the move to rustc?

@orium
Copy link
Member Author

orium commented Feb 24, 2019

I think it was. From quickly grepping rustc's repository it looks like this value is hardcoded to 100:

https://github.com/rust-lang/rust/blob/f47ec2ad5b6887b3d400aee49e2294bd27733d18/src/librustc/session/mod.rs#L1188

I didn't find any place where this field gets changed (https://github.com/rust-lang/rust/search?q=const_eval_stack_frame_limit).

Not sure about the other configurations (memory_size, step_limit).

@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2019

Hm. yes, they seem to have gotten lost. We should probably just remove that limitation entirely, just like we got rid of the memory limit.

@orium
Copy link
Member Author

orium commented Feb 24, 2019

Currently rustc uses this stack_limit = 100 when evaluating const fn.

Do we want to keep this limit for compilation?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2019

For const eval we can just use the recursion limit, which is already configurable by users

@RalfJung RalfJung added C-bug Category: This is a bug. A-interpreter Area: affects the core interpreter labels Mar 8, 2019
@RalfJung
Copy link
Member

The bug part of this is now "fixed" by removing this text from --help.

I am turning this into a ticket to track the enhancement request that the stack limit should be changeable.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement and removed C-bug Category: This is a bug. labels Mar 14, 2019
@RalfJung RalfJung changed the title Unable to change stack limit Should be able to change stack limit Mar 14, 2019
@CAD97
Copy link

CAD97 commented Nov 25, 2019

I think rust-lang/rust#66726 should address this?

I would just like to note that I hit the stack frame limit exactly, which prompted me to check how difficult it'd be to change this:

>cargo miri test
[...]
error: Miri evaluation error: reached the configured maximum number of stack frames
   --> D:\usr\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\src\libcore\mem\mod.rs:245:5
    |
245 |     intrinsics::size_of::<T>()
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: reached the configured maximum number of stack frames
    |

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interpreter Area: affects the core interpreter C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants