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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.