review (S) for 6970683: improvements to hs_err output

[Vladimir Kozlov]

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);
+      }

> 
>> 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).

> 
>> 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).

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.
>