[9] RFR(S): 8050079: crash while compiling java.lang.ref.Finalizer::runFinalizer

[Tobias Hartmann]

Hi Vladimir,

thanks for the review.

On 13.11.2014 20:48, Vladimir Kozlov wrote:
> Should you fix problem (2) other way around by adding check in the following
> loop?  If you don't bailout on 'nof_impls > 1' on interface what result you get?

According to the comment we want to avoid this case:

 *I.m > { A.m, C }; B.m > C

There are two cases were we may encounter an interface in the subclass hierarchy
and do not check for 'nof_impls > 1':
(1) context_type is java.lang.Object
(2) context_type is an interface

(1) If context_type is a java.lang.Object O it should be fine not to bailout if
we encounter an interface with two implementations. For example:

 O.m > *I > { A.m, C }; B.m > C

Coming from O we would miss the second implementation of I that is inherited
from B and overrides m. But since B also inherits from Object

 O.m > B.m

we catch B.m while traversing B and bailout because we now have two witnesses.
So no need to bailout early in this case.

(2) If context_type is an interface we may have the following hierarchy:

 *I.m > *I2 > { A.m, C }; B.m > C

Again, we would miss the the second implementation of *I.m inherited from B.m.
But since nof_impls == 2 for *I, we bailout even before reaching *I2.

So I don't think we have to move the check into the loop. What do you think?

> I think test should go into new 'dependencies' subdir not 'inlining'.

Okay, I moved it to compiler/dependencies/MonomorphicObjectCall/.

> Why you fork separate JVM process?

Because I need to set the -Xbootclasspath to the test classes directory to make
sure that the modified version of java.lang.Object is used. Specifying it in the
'@run main/othervm' command with

 '-Xbootclasspath/p:../classes/compiler/dependencies/TestMonomorphicObjectCall/'

depends on the location of classes and the working directory (fails on JPRT). I
solve this by reading the directory with 'System.getProperty("test.classes")'
and forking a separate JVM process. Another solution would be to use a shell
script but I prefer the more readable Java code.

New webrev: http://cr.openjdk.java.net/~thartmann/8050079/webrev.01/

Thanks,
Tobias

> 
> Thanks,
> Vladimir
> 
> On 11/13/14 6:51 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8050079
>> Webrev: http://cr.openjdk.java.net/~thartmann/8050079/webrev.00/
>>
>> Problem (1):
>> C1 compiles a call to Object.finalize() in
>> 'java.lang.ref.Finalizer::runFinalizer'. To determine if the call is
>> monomorphic, the compiler searches through the subclasses of Object until it
>> finds a class that overrides finalize(). This check is performed in
>> 'ClassHierarchyWalker::find_witness_anywhere' by calling 'is_witness(Klass* k)'
>> on the subclasses. After casting with 'InstanceKlass::cast(k)' the (Sub-)Klass k
>> is treated as an InstanceKlass and 'InstanceKlass::find_method' is called. The
>> crash happens in 'binary_search' when a subclass k is not an InstanceKlass but
>> an ObjectArrayKlass and therefore '_methods' is NULL.
>>
>> Why didn't the bug show up earlier?
>> The method Object.finalize() is usually overridden by the subclasses
>> 'java.lang.ClassLoader$NativeLibrary' and 'java.lang.Enum' which are loaded
>> during startup. That means a call to Object.finalize() is never treated as
>> monomorphic and 'ClassHierarchyWalker::find_witness_anywhere' returns before
>> encountering an ObjectArrayKlass in the subclass hierarchy of Object. The
>> failure only occurs in an internal test environment for JRebel [1]. According to
>> the core file of the crash, the usual class hierarchy of the Java Runtime
>> Environment was changed by the framework. For example, java.lang.String is no
>> longer a subclass of java.lang.Object but of another class called
>> 'JaveleonObject' [2]. Especially the two overriding classes
>> 'ClassLoader$NativeLibrary' and 'Enum' are not subclasses of Object. This leads
>> to the crash because C1 encounters an ObjectArrayKlass in the subclass hierarchy.
>> Of course, this bug could also show up if the sequence of subclasses in the
>> 'Klass::subklass()' list changes.
>>
>> Problem (2):
>> In case of a worklist overflow we recursively call 'find_witness_anywhere' and
>> create a new worklist (see line 1178 of dependencies.cpp). The problem is that
>> we now execute the interface related checks that would otherwise not be executed
>> (line 1132). For interface subtypes of Object (for example java/lang/Readable)
>> we then bail out if 'nof_impls > 1'.
>>
>> Solution:
>> (1) A check for 'oop_is_instance' is added to ignore non-instance Klasses in the
>> subclass hierarchy.
>> (2) I added a check to only execute the interface related checks if we come from
>> a top level call.
>>
>> I was finally able to write a small jtreg regression test. Although the test is
>> a little bit hacky, it is stable and deterministically reproduces the bug. The
>> test works by overriding the default implementation of java.lang.Object,
>> renaming the method 'finalize' to 'finalizeObject' and thus guaranteeing that
>> 'finalizeObject' is never overridden and all calls to it are monomorphic. By
>> triggering a C1 compilation of 'Object.finalizeObject()' we trigger the bug
>> because there are no overriding instance Klasses in the call hierarchy.
>>
>> The '-XX:-VerifyDependencies' flag is necessary to not bail out too early
>> because of problem (2).
>>
>> Testing:
>> JPRT with new test
>>
>> Thanks,
>> Tobias
>>
>>
>> [1] http://zeroturnaround.com/software/jrebel/
>> [2] https://bugs.openjdk.java.net/secure/attachment/23360/replay.log
>>