Fix edge case where rescued Exceptions may be nil #311

Merged
merged 1 commit into from Sep 26, 2012

Projects

None yet

3 participants

@bbrowning

If an inner rescue block causes a Java proxy or package to be looked
up then an outer rescue block was receiving a nil Exception object,
only when running in interpretered, non-IR mode. Running the test with
"-X+C" or "-X-CIR" did not reproduce the behavior.

This behavior was introduced by the commits 20632af and 7c3f642,
which were cleaning up a hack around the way $! got set. The mentioned
hack was covering up this bug.

@bbrowning bbrowning Fix edge case where rescued Exceptions may be nil
If an inner rescue block causes a Java proxy or package to be looked
up then an outer rescue block was receiving a nil Exception object,
only when running in interpretered, non-IR mode. Running the test with
"-X+C" or "-X-CIR" did not reproduce the behavior.

This behavior was introduced by the commits 20632af and 7c3f642,
which were cleaning up a hack around the way $! got set. The mentioned
hack was covering up this bug.
df7874f
@subbuss

Thanks Ben.

The reason -X+C doesn't have this bug is because I did not clean up the JIT to remove the $! hack as I did with -X-CIR and -X-C (See commit log note in 7c3f642). Curious why this example doesn't fail in -X-CIR mode since IR doesn't have the $! hack either. I do know that in IR mode, $! is saved and restored properly in IR-generated code, so maybe the clobbering in the runtime does not matter ... but just a guess.

The bigger issue that this bug exposes is that "$!" is a proxy for the errorInfo value in ThreadContext and so, $! can be modified indirectly by directly calling setErrorInfo. Ideally, in later commits, we should do the following:

  • clean up the JIT to get rid of the $! hack as done for the AST interp in 20632af
  • audit all uses of setErrorInfo to see if they clobber $! accidentally anywhere else.
  • unify the globalVariables.set("$!", foo) and setErrorInfo(foo) paths so that we use one or the other uniformly, if feasible.
@headius headius merged commit 4f2c44e into jruby:master Sep 26, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment