RFR(XS): JDK-6904403 assert(f == k->has_finalizer(), "inconsistent has_finalizer") with debug VM

[serguei.spitsyn at oracle.com]

On 5/28/14 6:37 AM, Coleen Phillimore wrote:
>
> Yes, this looks better.  Thanks!
++1
Reviewed.

Thanks,
Serguei

> Coleen
>
> On 5/28/14, 9:07 AM, Dmitry Samersoff wrote:
>> Serguei,
>>
>> Hopefully, I addressed all comments.
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-6904403/webrev.04/
>>
>> Please, notice changes in instanceKlass.cpp/instanceKlass.hpp
>>
>> -Dmitry
>>
>>
>> On 2014-05-28 00:07, serguei.spitsyn at oracle.com wrote:
>>> Dmitry,
>>>
>>> It looks good in general.
>>> Some minor comments below.
>>>
>>>
>>> On 5/27/14 8:45 AM, Dmitry Samersoff wrote:
>>>> Coleen,
>>>>
>>>> please see below.
>>>>
>>>> On 2014-05-27 19:02, Coleen Phillimore wrote:
>>>>> Dmitry,
>>>>> This is excellent.  Thank you for adding a test.  I have a couple
>>>>> comments but they are sort of simple things.
>>>>>
>>>>> 1.  In InstanceKlass::has_redefined_super() can you make "klass' an
>>>>> InstanceKlass* and cast to InstanceKlass on line 1491 instead to 
>>>>> avoid
>>>>> the casting.   You'll only have InstanceKlass on the way up.
>>>> InstanceKlass klass->super() return Klass *, so we have to cast it
>>>> anyway:
>>>>
>>>>       klass = InstanceKlass::cast(klass->super());
>>>>
>>>>    Current approach is a bit more readable on my view and similar 
>>>> to code
>>>> above.
>>> The approach suggested by Coleen is more readable, as it'd look like 
>>> this:
>>>
>>> bool InstanceKlass::has_redefined_super() const {
>>>    InstanceKlass* ik = this;
>>>    while (ik != NULL) {
>>>      if (ik->has_been_redefined()) {
>>>        return true;
>>>      }
>>>      ik = ik->java_super();
>>>    }
>>>    return false;
>>> }
>>>
>>> Compare with your current approach:
>>>
>>> bool InstanceKlass::has_redefined_super() const {
>>>    Klass* klass = const_cast<InstanceKlass*>(this);
>>>    while (klass != NULL) {
>>>      if (InstanceKlass::cast(klass)->has_been_redefined()) {
>>>        return true;
>>>      }
>>>      klass = InstanceKlass::cast(klass)->super();
>>>    }
>>>    return false;
>>> }
>>>
>>>
>>> I also think that the name "has_redefined_super" is a little bit 
>>> confusing.
>>> What about "has_redefined_this_or_super" ?
>>>
>>> Still reviewing the test...
>>>
>>> Thanks,
>>> Serguei
>>>>> 2.  The new test files need copyrights!   Except maybe the 
>>>>> manifest one
>>>>> (idk).
>>>> OK.
>>>>
>>>>> 3.  Can you make jtreg commands to do the things in the script?  I
>>>>> didn't see anything very unusual.
>>>> I tried it and ended up to bunch of java code instead of 10 lines of
>>>> straightforward shell script.
>>>>
>>>> jtreg compiles classes with tools.jar from COMPILEJAVA, not from
>>>> TESTJAVA. It might be a problem if COMPILE java doesn't exactly match
>>>> TESTJAVA (e.g. cross compilation) and I didn't find a way to change 
>>>> it.
>>>>
>>>> I didn't find a way to instruct jtreg to build a jar file required for
>>>> agent. I have to do it programatically and then use process builder to
>>>> launch one more VM.
>>>>
>>>> So I would prefer to stay with a shell here.
>>>>
>>>>> Thank you for doing this.  It's really helpful and it seems it 
>>>>> found a
>>>>> bug also (super classes?)
>>>> Initially this fix was intended as very quick band-aid to address jcov
>>>> problem important for SQE. But as soon as we go deeper I decide to 
>>>> make
>>>> better fix to cover other possible cases with redefined finalizers.
>>>>
>>>> -Dmitry
>>>>
>>>>> Coleen
>>>>>
>>>>> On 5/24/14, 1:17 PM, Dmitry Samersoff wrote:
>>>>>> Coleen,
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-6904403/webrev.03/
>>>>>>
>>>>>> This is new version of the fix.
>>>>>>
>>>>>> Changes:
>>>>>>
>>>>>> 1. Added testcase
>>>>>>
>>>>>> 2. Check entire class hierarchy for redefined classes
>>>>>>       as any of supers could be redefined and cause assert.
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>>> On 2014-04-16 16:56, Dmitry Samersoff wrote:
>>>>>>> Hi Everybody,
>>>>>>>
>>>>>>> Please review small changes.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-6904403/webrev.01/
>>>>>>>
>>>>>>> This fix tells to JVM to don't assert if the class was redefined.
>>>>>>>
>>>>>>> Please notice, these changes doesn't make redefined finalyzers
>>>>>>> working.
>>>>>>> It's a large project which is out of scope of this changes.
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>
>