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

[Coleen Phillimore]

Yes, this looks better.  Thanks!
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
>>>>>>
>