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

[Igor Ignatyev]

Vladimir,

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

Tobias,
0.
>   35  * @compile -Xbootclasspath/p:. java/lang/Object.java TestMonomorphicObjectCall.java
to allow compilation of Object, you should use '-XDignore.symbol.file'
1.
>   36  * @run main/othervm TestMonomorphicObjectCall
to decrease count of spawned processes, you should use agentvm mode 
instead of othervm.
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'

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