[9] RFR(S): 8164480: Crash with assert(handler_address == SharedRuntime::compute_compiled_exc_handler(..) failed: Must be the same

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 8/26/16 7:15 AM, Tobias Hartmann wrote:
> Hi Doug,
>
> On 26.08.2016 16:05, Doug Simon wrote:
>>
>>> On 26 Aug 2016, at 15:55, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8164480
>>> http://cr.openjdk.java.net/~thartmann/8164480/webrev.00/
>>>
>>> We crash in OptoRuntime::handle_exception_C_helper() while handling a java.lang.OutOfMemory error because the handler address obtained from the nmethod ExceptionCache does not match the address computed via SharedRuntime::compute_compiled_exc_handler().
>>>
>>> The underlying problem happens when the exception is first triggered and we compute the exception handler address to be stored in the exception cache for this pc. During computation, another OOM error is triggered in Method::fast_exception_handler_bci_for() during class loading and as a result we do another lookup for an exception handler for this new exception, this time starting at the bci of the exception handler which caused the exception to be thrown (see SharedRuntime::compute_compiled_exc_handler()). We return the handler address for the recursive OOM error which differs from the handler address for the original exception and therefore shouldn't be stored in the cache:
>>>
>>> 1355         // Update the exception cache only when the unwind was not forced
>>> 1356         // and there didn't happen another exception during the computation of the
>>> 1357         // compiled exception handler.
>>> 1358         if (!force_unwind && original_exception() == exception()) {
>>>
>>> However, the "original_exception() == exception()" check is not strong enough because OOM errors are preallocated and re-used in Universe::gen_out_of_memory_error() if PreallocatedOutOfMemoryErrorCount == 0. In this case, the oop of the original exception and the exception that happened during computation are equal and therefore we assume that no exception happened and put the incorrect handler address into the cache.
>>>
>>> I added a 'had_exception' argument to compute_compiled_exc_handler() which is set to true if a recursive exception curred. It seems like this check was completely missing in jvmciRuntime.cpp. I also added additional debug output to the assert.
>>
>> Yes, this was oversight in the JVMCI part of the code.
>
> Okay, thanks for verifying!
>
>> I would suggest the name recursive_exception_occurred instead of had_exception. Of course, any exception that occurs here is by definition recursive/nested but it does’t hurt to be crystal clear in this complex code.
>
> I agree. Here's the new webrev also including some additional comments:
> http://cr.openjdk.java.net/~thartmann/8164480/webrev.01/
>
> Thanks,
> Tobias
>
>>
>> -Doug
>>