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

Tue Nov 18 06:10:44 UTC 2014

Thanks Vladimir,

I'll add the test to JPRT.

Best,
Tobias

On 17.11.2014 18:30, Vladimir Kozlov wrote:
> 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
>>>>>>>