RFR: 8267118: OutOfMemoryError cannot be caught as a Throwable [v3]

[Coleen Phillimore]

On Tue, 1 Jun 2021 02:41:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This interesting little quirk was discovered by @iklam. During verification, the catch-type in a catch clause is normally resolved as part of being checked to see that it is a subclass of Throwable. At runtime when an exception is thrown, we also have to check if the exception being thrown is assignable to the catch-type and so handled by that catch block. For the case where the catch-type is Throwable itself, the verification subclass check trivially passes (due to a name match) without actually resolving the CP entry for the catch-type. So at runtime when the exception is thrown we have to perform the CP resolution of the catch-type. But the resolution process itself can trigger exceptions and in particular if we have thrown OutOfMemoryError, the resolution may throw a secondary OutOfMemoryError, which prevents the first from being caught by the catch block!
>> 
>> The fix is to force resolution of the catch-type at verification time, when it is Throwable.
>> 
>> To aid in debugging the original problem I've also added some new logging statements that show how we find a catch block and if we encounter further exceptions in trying to catch the exception - see example in the JBS issue.
>> 
>> Testing: tiers 1-3, plus the new test
>> 
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reworked the new logging to seperate Info and Debug levels to reduce verbosity, per
>   feedback from @coleenp @iklam

Can you reduce the amount of logging code please?

src/hotspot/share/oops/method.cpp line 232:

> 230:     ResourceMark rm(THREAD);
> 231:     log_debug(exceptions)("Looking for catch handler for exception of type \"%s\" in method \"%s\"",
> 232:                           ex_klass == NULL ? "NULL" : ex_klass->external_name(), mh()->name()->as_C_string());

You don't need () with mh().  just mh->

src/hotspot/share/oops/method.cpp line 261:

> 259:           ResourceMark rm(THREAD);
> 260:           log_info(exceptions)("Found catch-all handler for exception of type \"%s\" in method \"%s\" at BCI: %d",
> 261:                                ex_klass == NULL ? "NULL" : ex_klass->external_name(), mh()->name()->as_C_string(), handler_bci);

Same here.

src/hotspot/share/oops/method.cpp line 285:

> 283:         Klass* k = pool->klass_at(klass_index, THREAD);
> 284:         if (HAS_PENDING_EXCEPTION) {
> 285:         if (log_is_enabled(Debug, exceptions)) {

Technically, this would be interesting enough to be log_info level.

src/hotspot/share/oops/method.cpp line 300:

> 298:             log_info(exceptions)("Found matching handler for exception of type \"%s\" in method \"%s\" at BCI: %d",
> 299:                                  ex_klass == NULL ? "NULL" : ex_klass->external_name(), mh()->name()->as_C_string(), handler_bci);
> 300:           }

There's so much extra code added with this logging.  It's hard to follow as it is. Could you just have the log_info level output in both this and the case above where you find the handler, here and above?  debug level logging will include info level debugging.

-------------

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4266