review (S) for 6970683: improvements to hs_err output

[Tom Rodriguez]

On Oct 12, 2010, at 12:27 PM, Vladimir Kozlov wrote:

> Looks good.
> 
> One thing I notice is os_solaris_x86.cpp doesn't have' REG_' in reg names in 32-bit case but os_linux_x86.cpp has it. Why such difference?

As far as I know it's just a platform include difference.

Thanks!

tom

> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Oct 12, 2010, at 11:04 AM, Vladimir Kozlov wrote:
>>> Tom Rodriguez wrote:
>>>>> src/share/vm/runtime/os.cpp next values are the same why print twice?
>>>>> 
>>>>> +       st->print_cr(INTPTR_FORMAT " is the thread: "
>>>>> +                    INTPTR_FORMAT, addr, thread);
>>>> I wanted to print something shorter since we dump all the threads later but I didn't closely enough about what would come out.  How about this:
>>>> -      thread->print_on(st);                                                                                                                         +      if (verbose) {                                                                                                                                 +        thread->print_on(st);                                                                                                                       +      } else {                                                                                                                                       +        static char buf[256];                                                                                                                       +        thread->print_on_error(buf, sizeof(buf));                                                                                                   +        st->print_cr(INTPTR_FORMAT " is %s", addr, buf);                                 
>                                                           +      }   This will print out in the same one line format that the rest of VMError uses.
>>> It is too many lines (two :) ). I would just say it is thread, we can look it down in hs_err for more info.
>>> 
>>> -      thread->print_on(st);
>>> +      if (verbose) {
>>> +        thread->print_on(st);
>>> +      } else {
>>> +       st->print_cr(INTPTR_FORMAT " is the thread", addr);
>>> +      }
>>        st->print_cr(INTPTR_FORMAT " is a thread", addr);
>>>>> src/share/vm/code/codeCache.cpp closed ")" should be "]" (or it is indication that address in not included?):
>>>>> 
>>>>> +   st->print_cr("Code Cache  [" INTPTR_FORMAT ", " INTPTR_FORMAT ", " INTPTR_FORMAT ")",
>>>> It's the same format we use for the other sections and yes the final address isn't included.  It's the half open interval.
>>> Then it is fine.
>>> 
>>>>> also why you need to put ' ' around numbers for codecache values? Can you separate them using comma?
>>>> It was a copy paste from the log sweeper printing which was xml.  I've removed them.  Why do you want commas?
>>>> Code Cache  [0xfa800000, 0xfaa40000, 0xfd800000)
>>>> total_blobs=398 nmethods=299 adapters=60 free_code_cache=49158848
>>>> vs.
>>>> Code Cache  [0xfa800000, 0xfaa40000, 0xfd800000)
>>>> total_blobs=398, nmethods=299, adapters=60, free_code_cache=49158848
>>> I am fine without commas (less file size).
>> Ok.
>>>>> src/os_cpu/windows_x86/vm/os_windows_x86.cpp missing #ifdef AMD64 in os::print_register_info() and end of method:
>>>>> st->cr();
>>>>> }
>>>>> 
>>>>> Also why you left Register to memory dump the same? On solaris platforms you print only register name before call to print_location() (I prefer this).
>>>> Oops.  I thought I'd finished the windows side.  It's done now, though it hasn't been through jprt yet.
>>> I don't see that you updated print_register_info(), it is still original (several lines per reg).
>> I've updated the webrev.  There were also extra _cr's in the os_linux_x86 code that I removed.
>> tom
>>> Thanks,
>>> Vladimir
>>> 
>>>> tom
>>>>> Vladimir
>>>>> 
>>>>> Tom Rodriguez wrote:
>>>>>> http://cr.openjdk.java.net/~never/6970683
>>>>>> 6970683: improvements to hs_err output
>>>>>> Reviewed-by:
>>>>>> There are a few things missing from the hs_err dump that would be
>>>>>> useful.  First we don't dump the sparc L and I registers.  Second some
>>>>>> information about the size and contents of the code cache would be
>>>>>> useful.  Third we should dump a larger region around the faulting
>>>>>> instruction.  Additionally the new register to memory mapping output
>>>>>> can crash which stops us from getting the stack and instructions at
>>>>>> the faulting pc, so I moved it into it's own section.  block_start
>>>>>> would assert in some cases so I augmented existing logic to just
>>>>>> return null.  I also changed the formatting to remove all the extra
>>>>>> whitespace and made some of the output more compact and eliminated
>>>>>> most of the useless whitespace.