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

[Vladimir Kozlov]

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
>>>