review request for CR 6361589

[Vladimir Kozlov]

Volker,

Did you got a respond on this?

Thanks,
Vladimir

Volker Simonis wrote:
> 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
>>>>>>>>
>>>>>>>
>>