HSX-25: code review (round 0) for VMError::report_and_die () dies on bad PC (8015884)

[Daniel D. Daugherty]

Thanks for the quick re-review!

Dan


On 7/3/13 2:31 PM, Zhengyu Gu wrote:
> Looks good to me too.
>
> -Zhengyu
>
> On 7/3/2013 3:47 PM, Dmitry Samersoff wrote:
>> Dan,
>>
>> Looks good for me.
>>
>> -Dmitry
>>
>>
>> On 2013-07-03 22:37, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> Here are the (round 1) HSX-25 webrev URLs:
>>>
>>> OpenJDK: http://cr.openjdk.java.net/~dcubed/8015884-webrev/1-hsx25/
>>> Internal: 
>>> http://javaweb.us.oracle.com/~ddaugher/8015884-webrev/1-hsx25/
>>>
>>> Because of the whitespace HotSpot style changes, the "frames" version
>>> is the easiest to review... Yes, I've crawled through comparing the
>>> patch files from round 0 and round 1 and now I have a headache... :-)
>>>
>>> Changes relative to round 0:
>>>
>>> - additional changes to get proper hs_err_pid file generation in
>>> non-product
>>>    Win* bits when the following tests are executed:
>>>      -XX:ErrorHandlerTest=N option aka test_error_handler() function
>>>      -XX:+ExecuteInternalVMTests option aka execute_internal_vm_tests()
>>> function
>>>    - Coleen - you'll want to check out:
>>>        src/share/vm/prims/jni.cpp
>>>        src/os/windows/vm/os_windows.hpp
>>>        src/os/windows/vm/os_windows.inline.hpp
>>>
>>> - the latest version of test/runtime/6888954/vmerrors.sh now passes on
>>>    all platforms including MacOS X
>>>    - this includes test case 12 where we used to not get an hs_err_pid
>>>      file on Win* (just an exit code == 3)
>>>    - Gerard - please check out the new test and the changes in
>>>      src/share/vm/utilities/debug.cpp; we can discuss off-thread what
>>>      to do with: 7190125 runtime/6888954/vmerrors.sh fails on MacOS X
>>>
>>> - more code clean ups:
>>>    - redundant "else" removal - thanks Dmitry!
>>>    - switch to PTR_FORMAT in some print calls - thanks Harold!
>>>    - more HotSpot whitespace/style fixes... as long as I was in 
>>> there... :-)
>>>
>>> Of course, I'm looking for re-reviews from: Dmitry, Zhengyu,
>>> Harold and Coleen...
>>>
>>> Dan
>>>
>>>
>>> On 6/21/13 4:44 PM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have have a proposed fix for the following bug:
>>>>
>>>>      8015884 runThese crashed with SIGSEGV, hs_err has an error 
>>>> instead
>>>>              of stacktrace
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8015884
>>>>      https://jbs.oracle.com/bugs/browse/JDK-8015884
>>>>
>>>> Here are the HSX-25 webrev URLs:
>>>>
>>>> OpenJDK: http://cr.openjdk.java.net/~dcubed/8015884-webrev/0-hsx25/
>>>> Internal: 
>>>> http://javaweb.us.oracle.com/~ddaugher/8015884-webrev/0-hsx25/
>>>>
>>>> Testing:
>>>> -  test/runtime/6888954/vmerrors.sh has been updated to include a
>>>>     test for a bad function pointer
>>>> - vmerrors.sh has been used on MacOS X and Solaris X86 to verify
>>>>    that bad function pointers get better trouble shooting info
>>>> - (running, @ 24 hours and not yet done) JPRT build and test job
>>>> - (TBD) JPRT build and new test only job
>>>> - (TBD) Aurora Adhoc vm.quick batch for all platforms in the following
>>>>    configs: {Client VM, Server VM} x {fastdebug} x {-Xmixed}
>>>>
>>>> Just to be clear: This fix solves the problem of not getting a
>>>> stacktrace when we get a bad PC; it does not solve the underlying
>>>> reason why libpthread sent the VM into the weeds.
>>>>
>>>> Gory details are below. As always, comments, questions and
>>>> suggestions are welome.
>>>>
>>>> Dan
>>>>
>>>>
>>>> Gory Details:
>>>>
>>>> The primary bug fixes are:
>>>>
>>>> - the Dl_info struct should only be used if dladdr() has
>>>>    returned non-zero (no errors) and always check the dladdr()
>>>>    return value
>>>> - the Dl_info.dli_sname and Dl_info.dli_saddr fields should
>>>>    only be used if non-NULL
>>>> - the first two fixes are based on the Solaris dladdr() man page
>>>> - make logic consistent between MacOS X, Linux and Solaris:
>>>>    - offset is only set when a matching symbol is found (and
>>>>      the pointer is non-NULL)
>>>>
>>>> Cleanup bug fixes are:
>>>>
>>>> - add assert()'s to verify os::dll_address_to_function_name() and
>>>>    os::dll_address_to_library_name() contract for 'buf' parameter
>>>>
>>>> - HotSpot style: explicitly check int function return values
>>>>    rather that assume conversion to boolean, e.g.
>>>>
>>>>      if (dladdr(...)) {  =>  if (dladdr(...) != 0) {
>>>>
>>>> - consistently compare Dl_info fields against NULL instead of a mix of
>>>>    '0', NULL and implied boolean
>>>>
>>>> - the Dl_info.dli_fname and Dl_info.dli_fbase fields are only used if
>>>>    non-NULL; this was done for consistency (and paranoia); the Solaris
>>>>    man page does not state whether or not NULL is a permitted value 
>>>> for
>>>>    these fields
>>>>
>>>> Test changes:
>>>> - add case "13" for bad function pointers
>>>> - change VM-side of the test code so that case "12", the
>>>>    os::signal_raise(SIGSEGV) case, does not have to be last;
>>>>    this puts a ShouldNotReachHere() call after the test_num
>>>>    case statement so this test function will always crash
>>>>    the VM in non-product bits
>>>> - change VM-side of the test code to issue an ERROR message
>>>>    when an unexpected test_num value is passed
>>>>
>>>>
>>>>
>>
>