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

[serguei.spitsyn at oracle.com]

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