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

Interpreter broken on master when exiting naturally #12769

Closed
asterite opened this issue Nov 21, 2022 · 4 comments · Fixed by #12849
Closed

Interpreter broken on master when exiting naturally #12769

asterite opened this issue Nov 21, 2022 · 4 comments · Fixed by #12849
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:compiler:interpreter

Comments

@asterite
Copy link
Member

This is with Crystal master.

With this file:

# foo.cr
p 1

Run it with the compiled interpreter:

$ bin/crystal i foo.cr

Here's part of the output:

Crystal interpreter 1.7.0-dev [3973f44d5] (2022-11-16).
EXPERIMENTAL SOFTWARE: if you find a bug, please consider opening an issue in
https://github.com/crystal-lang/crystal/issues/new/
1
Invalid memory access (signal 11) at address 0x25
[0x102900d18] *Exception::CallStack::print_backtrace:Nil +104 in /Users/aryborenszweig/Projects/c
rystal/.build/crystal
[0x1028d7e14] ~procProc(Int32, Pointer(LibC::SiginfoT), Pointer(Void), Nil)@src/signal.cr:127 +320 in /Users/aryborenszweig/Projects/crystal/.build/crystal
[0x1ae18c4e4] _sigtramp +56 in /usr/lib/system/libsystem_platform.dylib
[0x1036e96ac] *Crystal::Repl::Interpreter#interpret<Crystal::ASTNode+, Crystal::Type+>:Crystal::Repl::Value +464004 in /Users/aryborenszweig/Projects/crystal/.build/crystal
...

Doing a git bisect, the first bad commit is 3973f44, which is this PR: #12608

I didn't investigate yet why that commit broke things... maybe there's an issue with arc4random in interpreted mode.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:interpreter labels Nov 21, 2022
@asterite
Copy link
Member Author

I have no idea what's going on but for now we could avoid using arc4random in interpreted mode until we figure it out.

I don't think this is related to arc4random though: if the hasher seed buffer isn't filled at all the same problem happens. No idea why.

@HertzDevil
Copy link
Contributor

I tried to do make clean crystal && git cherry-pick --no-commit 3973f44 and the first bad commit was 4abeeb1, which was #12405. Reverting just that commit on master also unbreaks the interpreter.

@asterite
Copy link
Member Author

Interesting. Then I think we should revert it. Such optimizations are good but are worthless if there's no way to do incremental or modular compilation. So it's better that things work correctly.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 17, 2022

Resolved by #12849

Further progress of the optimization is tracked in #12851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:compiler:interpreter
Projects
None yet
3 participants