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

[serguei.spitsyn at oracle.com]

Dmitry,

The test is nice, thank you for writing it!
Just several minor comments.

test/runtime/RedefineFinalizer/Agent.java

  It is easy to confuse the 'l0' with '10'. :)
  Could you, please, replace 'l0' with 'label', 'lab', or 'lab0'.
  (Lines: 23, 36, 48)

  Missed space after the comma:
    66         agentmain(args,inst);

  This definition is not really used:
    11     private static Instrumentation instrumentation;

  test/runtime/RedefineFinalizer/Main.java

   The indents are inconsistent:
     #2 - 4 spaces
     #3 - 3 spaces
     #4 - 6 spaces
     #8 - 5 spaces

    1 public class Main {
    2     public static void main(String[] args) {
    3        try {
    4              MartyrSon m = new MartyrSon();
    5              System.out.println(m.getName());
    6              System.runFinalization();
    7        } catch (Throwable e) {
    8             e.printStackTrace();
    9        }
   10     }

  test/runtime/RedefineFinalizer/MartyrSon.java

  The indent is incorrect, must be 4 instead of 3.


Thanks,
Serguei

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