RFR 8026324: hs_err improvement: Add summary section to hs_err file

[David Holmes]

Hi Coleen,

Looks good.

A few minor comments ...

On 17/07/2015 5:20 AM, Coleen Phillimore wrote:
> 8026333: hs_err improvement: Print GC Strategy
> 8026336: hs_err improvement: Print compilation mode, server, client or
> tiered
> Summary: Added command line, summary cpu and os information to summary
> section.  Moved time of crash and duration in summary section.  Add GC
> strategy and compiler setting (tiered) to enhanced version string in
> error report.  Moved the stack trace sooner in hs_err file.
>
> See new summary format in bug report and some samples.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8026324.01/
> bug link https://bugs.openjdk.java.net/browse/JDK-8026324
>
> Tested with RBT quick tests (tests that run on all platforms). Verified
> on crash logs on all platforms available inside Oracle either with
> crashing test or C++ program with same new code (or both), and various
> scenarios of missing cpuinfo/distribution files.

src/os/aix/vm/os_aix.cpp

+ // This looks good
+ void os::get_summary_cpu_info(char* buf, size_t buflen) {
+   // This looks good

It doesn't look _that_ good :)

---

src/os/bsd/vm/os_bsd.cpp

1709   size_t size = sizeof(mhz);
1710   int mib[] = { CTL_HW, HW_CPU_FREQ };
1711   size = sizeof(mhz);

Line #1711 is redundant

1713       mhz = 0;

Wouldn't a default of 1 be safer incase anyone parses this and does 
arithmetic?

BTW took me a while to realize these "if" bodies were setting defaults 
for when sysctl failed.

1721     strncpy(model, cpu_arch, sizeof(model));

where did cpu_arch come from??

---

src/os/linux/vm/os_linux.cpp

2066     if (file == NULL) break;
2068     if (_print_ascii_file(file, st)) return;

Can you put the break/return on a new line please.

More generally for some reason today I have an aversion to "if (x) 
foo();" being on a single line :) But control flow in particular I 
prefer to see on its own line.

2082     char buf[257];

Where does the 257 line length come from? Is this  a defined limit for 
the release file?

2218       char buf[257];

Where does the 257 line length come from? The flags line on my linux box 
is 410 characters.

2247   ARM32_ONLY("arm32") PPC_ONLY("PPC64") AARCH64_ONLY("AARCH64"), 
length);

May I suggest "ARM" for ARM32 and it should be "AArch64".

---

  src/os/posix/vm/os_posix.cpp

  241   // could use uname as in above also
  242   return (gethostname(buf, buflen) != -1);

I would suggest using uname given that Linux Glibc implements 
gethostname using uname. gethostname also has unclear semantics 
regarding nul-termination if the name is truncated.

---

src/os/solaris/vm/os_solaris.cpp

1978     char tmp[256];
1979     if (fgets(tmp, 256, fp)) {

256 this time? :) And the second 256 should sizeof(tmp).

Can you add a comment that you are getting only the first line, please.

2028 #ifdef AMD64
2029   snprintf(buf, buflen, "x86 64 bit %d MHz", clock);
2030 #elif  (defined __sparc) && (defined _LP64)
2031   snprintf(buf, buflen, "Sparcv9 64 bit %d MHz", clock);
2032 #else
2033 #error "Invalid solaris configuration"

I think "ifdef AMD64 else // must be sparc" will do here.

Aside: it is truly horrible how little useful processor info you can get 
from Solaris. It still refers to i386 and i387!

---

src/os/windows/vm/os_windows.cpp

1620   if (GetComputerNameEx(ComputerNameDnsHostname, buffer, &size)) {

Is it worth turning the above into a call to os::get_host_name to avoid 
the duplicate call?

---

src/share/vm/runtime/arguments.cpp

I'm not quite sure what gets printed for print_summary_on, or in what 
format. Can you given an example please showing where each piece comes 
from (jvm_flags vs jvm_args vs java_command).

---

src/share/vm/utilities/vmError.cpp

691   STEP(250, "(printing register info)")
699   STEP(260, "(printing registers, top of stack, instructions near pc)")

What's the difference between registers and register info ??

564   STEP(140, "(printing VM options)" )
816   STEP(380, "(printing VM options)" )

One of those should be changed.

Thanks,
David
-----

> Thanks,
> Coleen