HSX-25: code review (round 0) for VMError::report_and_die () dies on bad PC (8015884)
Dmitry Samersoff
dmitry.samersoff at oracle.com
Wed Jul 3 12:47:40 PDT 2013
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
>>
>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the hotspot-runtime-dev
mailing list