review (S) for 6970683: improvements to hs_err output

[Tom Rodriguez]

On Oct 7, 2010, at 2:03 PM, Vladimir Kozlov wrote:

> src/share/vm/utilities/vmError.cpp  change comment:
> 
> +   STEP(105, "(printing register info)")
> +
> +      // registers, top of stack, instructions near pc

fixed.

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

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

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


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

> 
> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp missing "=" after register name in print_register_info().

fixed.

> 
> src/os_cpu/linux_x86/vm/os_linux_x86.cpp print_register_info() should be as in os_solaris_x86.cpp

fixed.

> 
> src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp missing print_register_info() method.

Added, though not tested.

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.