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

[Tobias Hartmann]

Vladimir, Igor, thanks for the review.

>> I think ClassFileInstaller will do that.
> unfortunately, ClassFileInstaller can't help w/ it. because of classloader
> delegation model, ClassFileInstaller always finds the original Object class
> instead of custom one.

Yes, I've tried that while writing the test.

> > Tobias,
> 0.
>>   35  * @compile -Xbootclasspath/p:. java/lang/Object.java
>> TestMonomorphicObjectCall.java
> to allow compilation of Object, you should use '-XDignore.symbol.file'

I removed '-Xbootclasspath/p:.' and added '-XDignore.symbol.file'.

> 1.
>>   36  * @run main/othervm TestMonomorphicObjectCall
> to decrease count of spawned processes, you should use agentvm mode instead of
> othervm.

I changed the code to '@run main'.

> 2.
>>   48     static public void main(String[] args) throws Throwable {
>>   58     static public void executeTestJvm() throws Throwable
> just a code style, you have disordered method modifiers. the right order is
> 'public static void'

Fixed.

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

Thanks,
Tobias

> 
> Igor
> 
> On 11/14/2014 11:41 PM, Vladimir Kozlov wrote:
>> Tobias, you are right. I missed that B is inherited from Object too and
>> we process it anyway. Please, address John's comment.
>>
>> And we need sqe help with test. I want to run it without forking process
>> by installing modified Object.class into working directory so we can use
>> -Xbootclasspath/p:.
>>
>> I think ClassFileInstaller will do that.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/14/14 3:17 AM, Tobias Hartmann wrote:
>>> 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
>>>>>