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

Fix fncall #14

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Fix fncall #14

merged 1 commit into from
Aug 8, 2024

Conversation

yutannihilation
Copy link
Contributor

As you probably already notice, the current dev branch fails to process statefn2_same.mmm. This pull request is an attempt to fix this by partly reverting the change introduced in #3.

thread 'cpal_wasapi_out' panicked at library\core\src\panicking.rs:221:5:
unsafe precondition(s) violated: slice::get_unchecked requires that the index is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
error: process didn't exit successfully: `target\debug\mimium-cli.exe mimium-lang/tests/mmm/statefn2_same.mmm` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

The stack trace indicates the panic occurs here:

https://github.com/tomoyanonymous/mimium-rs/blob/f2bb45ec4d4c46ffc05d4b54f72587005ea35a9f/mimium-lang/src/runtime/vm.rs#L36

It seems rawdata is resized by the function's state_size.

https://github.com/tomoyanonymous/mimium-rs/blob/f2bb45ec4d4c46ffc05d4b54f72587005ea35a9f/mimium-lang/src/runtime/vm.rs#L625

The state_size is the sum of statesize supplied from the caller of emit_fncall().

https://github.com/tomoyanonymous/mimium-rs/blob/f2bb45ec4d4c46ffc05d4b54f72587005ea35a9f/mimium-lang/src/compiler/mirgen.rs#L272-L274

So, my guess is Instruction::PushStateOffset() and push_sum should use statesize instead of the offset from the context. But, honestly I'm not confident if my understanding is correct...

@tomoyanonymous tomoyanonymous added the bug Something isn't working label Aug 7, 2024
@tomoyanonymous
Copy link
Collaborator

Hmm, I have not noticed this because statefn2_same.mmm passes the test on my environments(M1 Mac & WSL2). Maybe the check of unsafe precondition is more strict on Windows?
Anyway, this partial revert works without problem on my environment.
This implementation of internal state size calclation is still complicated for me, we need to figure out more robust implmentation without using destructive assignment directly

@yutannihilation
Copy link
Contributor Author

Oh, you didn't notice this? (I thought this is the reason why dev branch is not merged into main yet.)

Maybe the check of unsafe precondition is more strict on Windows?

Sorry, I didn't describe the details. This check is an additional one which is enabled only on debug build. I found this error on cargo test, but this doesn't show up if I run the file with cargo run --release.

This implementation of internal state size calclation is still complicated for me, we need to figure out more robust implmentation without using destructive assignment directly

Thanks. I hope we can figure it out soon!

@yutannihilation yutannihilation mentioned this pull request Aug 7, 2024
@tomoyanonymous tomoyanonymous merged commit 0ad3b62 into mimium-org:dev Aug 8, 2024
@yutannihilation yutannihilation deleted the fix-fncall-offset branch August 8, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants