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

setjmp does not restore local variables #747

Closed
xxuejie opened this Issue Dec 7, 2012 · 12 comments

Comments

Projects
None yet
2 participants
@xxuejie
Contributor

xxuejie commented Dec 7, 2012

Please refer to this gist: https://gist.github.com/4237169

The value of local variable prev_jmp is not restored to what it was before the setjmp call, so the result is an infinite loop.

What's more, the C++ exception also suffers from this problem, I created another gist describing this: https://gist.github.com/4237412

Thanks for your help.

@kripken kripken closed this in 2699ec8 Dec 8, 2012

@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

Thanks for reporting! Fixed and added this testcase for longjmp.

kripken added a commit that referenced this issue Dec 8, 2012

@xxuejie

This comment has been minimized.

Contributor

xxuejie commented Dec 8, 2012

Thanks @kripken for the quick fix! However, I think this fix may bring another problem: we cannot call longjmp multiple times on the same setjmp, I prepared a gist at here: https://gist.github.com/4238475

Can you also take a look at this? Thanks again for your help!

kripken added a commit that referenced this issue Dec 8, 2012

rewrite setjmp code to identify, uniquely, each setjmp and match it t…
…o a longjmp. add testcase for #747, works in unoptimized builds
@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

Ok, after the last commit all tests pass, including this one - but only in unoptimized builds. In optimized builds, it fails even natively, that is, when compiling with clang -O2 to a native binary! So either there is a bug in clang, or this is undefined behavior. Do you know if it is defined?

@xxuejie

This comment has been minimized.

Contributor

xxuejie commented Dec 8, 2012

Sorry to reply this so late, I'm in Eastern Time Zone and I didn't see this until I woke up this morning. I checked the C99 spec, this is not mentioned in any part of it. So I suppose this may be an undefined behavior.

Actually this test case was taken from mruby source code. However, when I use clang with -O2 to compile mruby, it works without any problems. I guess there may still be some minor differences between this test case and the original mruby source. Anyway, I will look into this and provide you with more information when I figure this out. Sorry for the trouble caused

@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

Btw, another interesting thing to check is a native build with LLVM 3.2 (I think there is a release candidate out, or soon will be). If this is a bug in LLVM, perhaps it's fixed in 3.2.

@kripken kripken reopened this Dec 8, 2012

kripken added a commit that referenced this issue Dec 8, 2012

properly use identifier given to resume instruction, avoids issues wi…
…th cxa_catch cleaning it up; fixes test_multiexception, the exception-handling part of #747
@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

I fixed the exception testcase. The issue was unrelated - we did not implement the LLVM resume instruction properly, and we didn't have enough testing for resuming exceptions.

So that leaves just the testcase that fails in clang -O2 native builds, that we need to figure out if it is undefined behavior or a bug in clang.

@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

Actually the exception testcase is not fully fixed. Still investigating.

@xxuejie

This comment has been minimized.

Contributor

xxuejie commented Dec 8, 2012

Thanks for your update, here's what I found:

There is evidence that the mruby source did invoke two longjmp calls on one jmp_buf, and the native builds using gcc and clang (with either -O2 or -O3 options) both work. However, the same testcase in my gist runs neither on gcc or on clang. I'm still trying to come up with a test case that both invokes two longjmp calls and works on a native build.

@xxuejie

This comment has been minimized.

Contributor

xxuejie commented Dec 8, 2012

Now I got the original test case work when compiling with -O2 option. The modified source code is at https://gist.github.com/4242509. The only change I made is at line 13 of setjmp-twice.c. I added a "volatile" keyword here. I should've think of this earlier.

It also seems the head of incoming branch can handle this case.

I guess the reason may be that mruby has a much deeper calling stack between the location where setjmp is called and where longjmp is called. With our so short test case, the compiler can do a lot of optimizations within the two functions involved, which may bring the problem.

kripken added a commit that referenced this issue Dec 8, 2012

better fix for the exception testcase in #747 - set the current excep…
…tion when we are resuming after an end-catch which wiped it out, but not otherwise

@kripken kripken closed this in 0390d49 Dec 8, 2012

@kripken

This comment has been minimized.

Owner

kripken commented Dec 8, 2012

Thanks! That makes sense actually - this must be undefined behavior, we are jumping back to a previous point in the same function, and changing a variable that by simple control flow analysis must be constant.

Looks like we have all the cases here working now, I updated the test.

@xxuejie

This comment has been minimized.

Contributor

xxuejie commented Dec 8, 2012

Thank you for all your help! And sorry I missed the "volatile" part in the first place. Now it seems we got a working setjmp/longjmp:)

@kripken

This comment has been minimized.

Owner

kripken commented Dec 9, 2012

Yes :)

And thanks for the great testcases! Very helpful with this kind of stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment