morganstanley / hobbes

A language and an embedded JIT compiler
http://hobbes.readthedocs.io/
Apache License 2.0
1.17k stars 105 forks source link

Core Dump when binding a function which return the fileref value. #368

Closed dawa79 closed 4 years ago

dawa79 commented 4 years ago

With the PullRequest #352 , it brings a new issue, the test case is like:

1) create 2 stream in slog file, call "test" and "testdup". in testdup it just save the string ref of the same string in "test" 2) bind 2 function for TestCase::save and TestCase::dup, TestCase::save return the strref pointer 3) Call a hobbes function "printRef“ to print the strref::index , this value seems the wrong value 4) duplicate the string created by step 2), it destroy the slog file. 5) if we don't not call printRef, everything is OK. in printRef, we only convert it to long type value.

any idea on this issue?

here is the test case code:

test.txt

dawa79 commented 4 years ago

One more thing is strref save(hobbes::array* a) if the return strref type not pointer type, it will make coredump.

kthielen commented 4 years ago

It seems like you've done a lot of work to narrow down a problem, which I'd be happy to look into. Your program here doesn't compile though, could you rework it so that it does compile and reproduce the problem you're seeing? If you're hitting a problem getting your test program to compile, I can help with that too (just let me know what problems you hit and I will explain).

dawa79 commented 4 years ago

I update the a file. it should be ok. you can check the slog file: /var/tmp/test.log. the stream testdup has been destroyed

dawa79 commented 4 years ago

Does that program compile for you?

Yes.

kthielen commented 4 years ago

Darn, I see what's going on. I should have included a check for this when I introduced this fileref type. I will post an update shortly.

kthielen commented 4 years ago

Sorry about this Dan, I hope it hasn't caused you too much stress. It looks like this change affected how fileref values are passed in return values. I fixed this, and verified the expected behavior with a test (first verifying that the test failed prior to my fix): https://github.com/Morgan-Stanley/hobbes/pull/369

kthielen commented 4 years ago

By the way, it might not be obvious from the content of the PR, but these fileref values are meant to be returned by value rather than by reference. It's a slightly dangerous game doing this, because although gcc will always pass a naked int64 in a register, it might decide not to do that for structs wrapping an int64 (as in this case).

The only reason we do that wrap here is to signify the "intent" of the int64 (that it "means" an offset into a file where a T is stored).

So that part of the binding can be simplified at least, and the fundamental problem here was that gcc decided it'd be better to rewrite fileref f(x) into void f(fileref*, x) but our interpreter/compiler didn't know about that decision. In this case a simple test can verify that at least, so I added it.

dawa79 commented 4 years ago

Thanks for quick fix!