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

Mon Nov 17 17:30:44 UTC 2014

Okay, looks good.

If it is a short test, please, add compiler/dependencies to TEST.groups so that it runs in JPRT (no need re-review).

Thanks,
Vladimir

On 11/17/14 3:33 AM, Tobias Hartmann wrote:
> 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
>>>>>>