Review Request: 8006506: Add test for redefining methods in backtraces to java/lang/instrument tests

[Stefan Karlsson]

On 2013-02-01 12:48, David Holmes wrote:
> Hi Stefan,
>
> This part of the test:
>
>   86     private static void doClassUnloading() {
>   87         // This will clean out old, unused redefined methods.
>   88         System.gc();
>   89     }
>
> seems to make assumptions about System.gc() and class unloading. Are 
> we relying on knowledge of hotspot internals here?
Yes, we do. This is a regression test trying to provoke one very 
specific bug in HotSpot, so I figured that would be OK. I don't know any 
JVM agnostic way to do this.
>
> I don't have a good suggestion to avoid this. This topic came up on 
> another thread today regarding weak references. For that case we can 
> typically add a loop and sleep until we see the reference clear, but 
> here ... how can you check that class unloading occurred? Actually is 
> that really what is happening - I don't see how any class unloading 
> will occur here. I can imagine that GC might cleanup unreferenced 
> methods.

I guess the name is not strictly correct, but we do the cleanup from the 
do_unloading code:
  ClassLoaderData::free_deallocate_list()
   ClassLoaderDataGraph::do_unloading
    SystemDictionary::do_unloading

StefanK

>
> David
>
> On 1/02/2013 9:27 PM, Stefan Karlsson wrote:
>> On 2013-02-01 12:17, Stefan Karlsson wrote:
>>> On 2013-02-01 12:11, serguei.spitsyn at oracle.com wrote:
>>>> On 2/1/13 1:57 AM, Stefan Karlsson wrote:
>>>>> On 2013-02-01 10:22, serguei.spitsyn at oracle.com wrote:
>>>>>> Nice test!
>>>>>> It looks good.
>>>>> Thanks for reviewing!
>>>>>>
>>>>>> As the original bug and the test are non-trivial, it'd make sense
>>>>>> to add a comment to
>>>>>> the class RedefineMethodInBacktraceApp and explain a little bit
>>>>>> what the test is doing,
>>>>>> and what behavior is expected.
>>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.04/
>>>>>
>>>>> Tell me if you think this is good enough.
>>>>
>>>> It is good.
>>>>
>>>> Nit: it'd be enough if it is more specific. :)
>>>> This method is a key point:
>>>>    90     private static void 
>>>> touchRedefinedMethodInBacktrace(Throwable throwable) {
>>>>    91         throwable.getStackTrace();
>>>>    92     }
>>>> Is it true that the test expects the getStackTrace() does not crash 
>>>> nor
>>>> throw an exception which would happen if the old/obsolete method is
>>>> gc'ed?
>>>
>>> I see. I'll add a comment that we shouldn't crash.
>>
>> http://cr.openjdk.java.net/~stefank/8006506/webrev.05/
>>
>> StefanK
>>
>>>
>>> Thanks,
>>> StefanK
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> thanks,
>>>>> StefanK
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Sergueri
>>>>>>
>>>>>>
>>>>>> On 2/1/13 12:13 AM, Stefan Karlsson wrote:
>>>>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.03/
>>>>>>>
>>>>>>> 1) Reverted the ProblemList change, since the fix has already
>>>>>>> propagaged to jdk8/tl
>>>>>>> 2) Renamed do_redefine -> doRedefine
>>>>>>> 3) Updated the .sh file with the bug number of the original CR
>>>>>>> instead of the test CR.
>>>>>>>
>>>>>>> thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>> On 2013-01-22 14:11, Stefan Karlsson wrote:
>>>>>>>> http://cr.openjdk.java.net/~stefank/8006506/webrev.00/
>>>>>>>>
>>>>>>>> This test provokes the JVM crash described in bug: JDK-7174978.
>>>>>>>>
>>>>>>>> I intend to push this to:
>>>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> StefanK
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>