review request for CR 6361589

[Yumin Qi]

  Hi, all

   Somehow I missed this email. Yes, Volker is right, this part miss a 
check if _thread is valid.
   We need to fix it under a bug number(even it is a very simple fix). I 
will file a bug for it.

Thanks
Yumin

On 2010/10/6 11:50, Vladimir Kozlov wrote:
> 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
>>>>>>>>>
>>>>>>>>
>>>