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

CoreRegs::default() causes stack overflow #40

Closed
6293 opened this issue Jan 10, 2022 · 2 comments · Fixed by #41
Closed

CoreRegs::default() causes stack overflow #40

6293 opened this issue Jan 10, 2022 · 2 comments · Fixed by #41
Labels
bug Something isn't working *portability* Issues affecting VM portability upstream Issues blocked by changes that has to be implemented/accepted at upstream
Milestone

Comments

@6293
Copy link
Contributor

6293 commented Jan 10, 2022

Current Behavior

#[test]
fn my_test() { let _register = CoreRegs::default(); }

fails with

thread 'isa::exec::tests::my_test' has overflowed its stack
fatal runtime error: stack overflow

Cause

https://github.com/internet2-org/rust-aluvm/blob/a7e64abd4a7058bf9f06886452147810b4a5cf6d/src/reg/core_regs.rs#L99
With Box<[LibSite; CALL_STACK_SIZE]>, the value would be allocated to stack first, but CALL_STACK_SIZE = 1 << 16 is too big, resuting in overflow.

Possible Solutions

Use Vec<LibSite> instead.

Related Issues

rust-lang/rust#53827

@6293
Copy link
Contributor Author

6293 commented Jan 10, 2022

It is a bit weird that
https://github.com/internet2-org/rust-aluvm/blob/a7e64abd4a7058bf9f06886452147810b4a5cf6d/src/isa/asm.rs#L39 does not fail (because Vm::<Instr>::with calls CoreRegs::default()), but when I modified CALL_STACK_SIZE to 1 << 18, this also overflowed. So most likely 1 << 16 is near the boundary

@dr-orlovsky
Copy link
Member

This is bad - and caused by rust compiler bug. It may be changed with stabilizing alternative box syntax, but nobody knows when (and would) it happen.

The worst is that this limits for the size are platform-dependent and will overflow on different platforms at different boundaries...

Vec seems to be the only solution here, you are right.

@dr-orlovsky dr-orlovsky added *portability* Issues affecting VM portability bug Something isn't working upstream Issues blocked by changes that has to be implemented/accepted at upstream labels Jan 10, 2022
@dr-orlovsky dr-orlovsky added this to Backlog in AluVM via automation Jan 10, 2022
@dr-orlovsky dr-orlovsky added this to the 0.5.0 milestone Jan 10, 2022
AluVM automation moved this from Backlog to Done Jan 11, 2022
@dr-orlovsky dr-orlovsky removed this from Done in AluVM Nov 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working *portability* Issues affecting VM portability upstream Issues blocked by changes that has to be implemented/accepted at upstream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants