review request for CR 6361589

[Vladimir Kozlov]

We should fix it in HS19 also. If we do it this week it will be ready
for next promoted build of 6u23.

Thanks,
Vladimir

Yumin Qi wrote:
>  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
>>>>>>>>>>
>>>>>>>>>
>>>>
>