RFR 8026324: hs_err improvement: Add summary section to hs_err file
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Mon Jul 20 20:26:53 UTC 2015
Hello Coleen,
Few comments from me:
src/os/linux/vm/os_linux.cpp
In parse_os_info function it is possible that file will not be
closed(if 'ptr==NULL' on line 2095). I think that "fclose(fp);" can be
added after 2110 line. Also, in this case lines 2100-2101 and 2108-2109
are seems reduntant, because file will be closed by added "fclose(fd);"
and function will terminate.
In os::get_summary_cpu_info function it is possible that file will
not be closed(if 'ptr' always == 'end' on line 2238). I think that
"fclose(fp);" can be added after 2248 line.
src/share/vm/runtime/os.cpp
In os::print_summary_info function you print physical_memory in
gigabytes. But on systems with less than 1 Gb of memory it will print 0.
Probably will be better to print it in megabytes?
Thank you,
Dmitry
On 17.07.2015 22:16, Coleen Phillimore wrote:
>
> Hi David, Thank you for taking the time to look and make comments on
> all of this. I have a new webrev at:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8026324.02/
>
> more below.
>
> On 7/17/15 3:14 AM, David Holmes wrote:
>> 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 :)
>
> Oh, yeah, the second one should say it looks "awesome". I took out
> one of the comments. The AIX guys can contradict me if they want.
>>
>> ---
>>
>> 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?
>
> Hm, maybe I shouldn't report it if it's not available instead.
>
>>
>> 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??
>
> Above... linux has a global cpu_arch too but different...
>>
>> ---
>>
>> 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.
>
> I was trying to make this look small. Ok. I don't really like one
> line if statements either anymore.
>
>>
>> 2082 char buf[257];
>> j
>> 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.
>
> I found 257 in another place and thought it seemed good enough for the
> cpuinfo line, because it's not looking for the 'flags' line. I wonder
> if there's an advantage to a power of two for a local stack buffer? I
> could go to 512 but I'm trying not to use stack space.
>
>>
>> 2247 ARM32_ONLY("arm32") PPC_ONLY("PPC64") AARCH64_ONLY("AARCH64"),
>> length);
>>
>> May I suggest "ARM" for ARM32 and it should be "AArch64".
>
> Okay.
>>
>> ---
>>
>> 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.
>
> Okay, I'll use uname. I suppose both make an OS call (wasn't sure if
> gethostname did).
>>
>> ---
>>
>> 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).
>
> Yes, I don't need a lot of characters. 256 should do. I'll change
> the second to sizeof.
>>
>> Can you add a comment that you are getting only the first line, please.
>
> ok.
>
>>
>> 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.
>
> Ok.
>>
>> Aside: it is truly horrible how little useful processor info you can
>> get from Solaris. It still refers to i386 and i387!
>>
>
> Yes, you think it would give me something better after going through
> the trouble of calling a syscall to get it. You get better from
> /proc/cpuinfo on linux but I don't know how to find this string on
> solaris.
>
>> ---
>>
>> 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?
>
> Yeah, that's nicer.
>>
>> ---
>>
>> 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).
>
> The distinction between jvm_flags and jvm_args is not obvious. The
> jvm_flags come from .hotspotrc (or settings file) and the jvm_args is
> what you'd actually pass on the command line or environment
> variable. I was going to add the flags to the command line but
> they're specified oddly without XX, which I guess you have discussed
> on another thread. I changed this summary printing to add a line for
> the contents of the settings file:
>
> --------------- S U M M A R Y ------------
>
> Settings File: +UseSharedSpaces +PrintCompressedOopsMode
> Command Line: -Xint Kaboom
>
> Host: mymachine, Intel(R) Core(TM) i5-3550 CPU @ 3.30GHz, 4 cores, 7G,
> Ubuntu 14.04.1 LTS
> Time: Fri Jul 17 15:04:33 2015 EDT elapsed time: 0 seconds (0d 0h 0m 0s)
>
>
> And I added comments.
>
> This might need some adjustment for bug 8061999 if they're not by
> default added to the command line arguments. I have to check that for
> that change.
>> ---
>>
>> 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 ??
>
> The first call decodes the register values if possible, ie. calls
> print_location() on each value. The second prints the registers as
> part of the context (which also includes top of stack and instructions
> near the pc. The second printing of the registers is redundant, and
> os::print_context() is only called from the vmError handler. I
> couldn't decide to remove it though, because if print_location() fails
> in step 250, you wouldn't have the registers, so you get them again in
> step 260.
>
>>
>> 564 STEP(140, "(printing VM options)" )
>> 816 STEP(380, "(printing VM options)" )
>>
>> One of those should be changed.
> Fixed.
>
> Thank you for noticing and suggesting all the things to clean up.
>
> Coleen
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks,
>>> Coleen
>
More information about the hotspot-dev
mailing list