review request for CR 6361589

[Volker Simonis]

Hi Yumin,

I know that this CR has already been submitted, but I've discovered a
small problem which I think should be fixed. I vmError.cpp you don't
check for '_thread' beeing NULL which can lead to a SIGSEGV if the
VMError object was constructed with a zero thread argument. Instead,
the corresponding if-line should read like the if-lines of the other
steps which use '_thread':

  STEP(135, "(printing target Java thread stack)" )

     // printing Java thread stack trace if it is involved in GC crash
     if (_verbose && _thread && (_thread->is_Named_thread())) {

Thank you and best regards,
Volker

On Fri, Dec 11, 2009 at 6:10 AM, Yumin Qi <Yumin.Qi at sun.com> wrote:
> Thanks.
>
> I modified the code and submitted again. Please take a look again, will
> commit if it is OK.
>
> Thanks
> Yumin
>
> Coleen Phillimore wrote:
>>
>> Yumin,
>> I've already reviewed a previous version and this still looks great to me.
>>  I hope this makes diagnosing crashes during gc much better, rather than
>> having to decode the thread manually as you've been doing for years.
>> Thanks everyone for the additional comments.  The later suggested changes
>> are really good.
>>
>> Coleen
>>
>> Yumin Qi wrote:
>>>
>>> Thanks for you all the good suggestions:
>>>
>>> 1) update the hierarchy comments --- we miss some there
>>> 2) Use c'tor-d'tor idiom for remembering JavaThread  in  GC thread
>>> 3) Add one more STEP (135)
>>> 4) update vmStructs so the SA can check this value.
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>>
>>> David Holmes - Sun Microsystems wrote:
>>>>
>>>> Hi Yumin,
>>>>
>>>> Can you update the thread hierarchy comments in thread.hpp:
>>>>
>>>>  49 // Class hierarchy
>>>>  50 // - Thread
>>>>  51 //   - VMThread
>>>>  52 //   - JavaThread
>>>>  53 //   - WatcherThread
>>>>
>>>> This seems incomplete anyway so it would be good to update it.
>>>>
>>>> I would also find it useful to add more comments to the NamedThread
>>>> class explaining the role of this new field. It really has nothing to do
>>>> with being a "named thread" - but then NamedThread should really have been
>>>> called something more general. Pity we can't rename it - can we? :)
>>>>
>>>> In vmError.cpp:
>>>>
>>>> 506      // printing Java thread stack trace if it is involved in GC
>>>> crash
>>>>  507
>>>>  508      if (_verbose && (_thread->is_Named_thread())) {
>>>>
>>>> Can/Should this be a new STEP?
>>>>
>>>> David
>>>>
>>>> Yumin Qi said the following on 12/11/09 08:50:
>>>>>
>>>>> Tom,
>>>>>
>>>>>  Thanks, changed as pointed.
>>>>>  Sorry I forget to include GC group.
>>>>>
>>>>>  Can someone else give a look?
>>>>>
>>>>> http://cr.openjdk.java.net/~minqi/6361589
>>>>>
>>>>>
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>
>>>>> Tom Rodriguez wrote:
>>>>>>
>>>>>> Overall this looks pretty nice.
>>>>>>
>>>>>> java_thread_gc isn't a very clear name.  Maybe collected_thread or
>>>>>> processed_thread?  The comment should describe it's purpose more explicitly.
>>>>>>
>>>>>> Is this assert really needed?
>>>>>>
>>>>>> +    assert(thread != this, "Should not be java thread itself");
>>>>>>
>>>>>> I think you should add is_NamedThread() instead of distinguishing all
>>>>>> these cases individually.
>>>>>>
>>>>>> If VMThread is now a NamedTread why don't you call set_name instead of
>>>>>> having it override name again?
>>>>>>
>>>>>> vmError.cpp:
>>>>>>
>>>>>> I think the printing output should look more like this:
>>>>>>
>>>>>> +         st->print_cr("JavaThread " PTR_FORMAT " was being processed
>>>>>> during the crash, id = " UINTX_FORMAT, jt, jt->osthread()->thread_id());
>>>>>>
>>>>>> tom
>>>>>>
>>>>>> On Dec 9, 2009, at 2:49 PM, Yumin Qi wrote:
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Fixed 6361589: Print out stack trace for target thread of GC crash
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~minqi/6361589
>>>>>>>
>>>>>>> Comments: This is a facility to assist for JVM crash analysis. To
>>>>>>> implement this function, it needs record JavaThread which may involve crash
>>>>>>> somewhere in GC thread. The place chosen here is NamedThread from which all
>>>>>>> GC threads but VMThread derive. I change VMThread's base from Thread to
>>>>>>> NamedThread, so the recording of JavaThread covers for GC threads and
>>>>>>> VMThread. Once the JavaThread recorded in GC thread seen during error
>>>>>>> report, its stack trace be printed out. To test this function, add a debug
>>>>>>> flag CrashGCForDumpingJavaThread, once it is set, it will crash immediately
>>>>>>> in VMThread or GC Thread to core dump. Note this is for test only purpose,
>>>>>>> no defects in involved JavaThread.
>>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>
>