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

[Dmitry Samersoff]

Dan,

> but I try not to change existing style unless I have to
> make other logic changes to the line.

os_linux.cpp:

1712 doesn't have redundant else, but 1795 does have.

I would prefer to have the same stile in all similar
code fragments if possible.

-Dmitry


On 2013-06-27 20:18, Daniel D. Daugherty wrote:
> Dmitry,
> 
> Thanks for the review!
> 
> Sorry for the delay in responding. I was out of the office
> for a couple of days.
> 
> Replies embedded below.
> 
> 
> On 6/22/13 11:51 AM, Dmitry Samersoff wrote:
>> Dan,
>>
>> os_linux.cpp:
>>
>> 1685:
>>    It might be better to just turn assert to guarantee. Something bad is
>> happening if we can't obtain JVM base address.
> 
> So that would be:
> 
> In os_bsd.cpp:
> 
> 1232 bool os::address_is_in_vm(address addr) {
> 1240     assert(libjvm_base_addr !=NULL, "Cannot obtain base address for
> libjvm");
> 
> In os_linux.cpp:
> 
> 1680 bool os::address_is_in_vm(address addr) {
> 1688     assert(libjvm_base_addr !=NULL, "Cannot obtain base address for
> libjvm");
> 
> In os_solaris.cpp:
> 
> 1922 bool os::address_is_in_vm(address addr) {
> 1930     assert(libjvm_base_addr !=NULL, "Cannot obtain base address for
> libjvm");
> 
> In os_windows.cpp:
> 
> 1477 bool os::address_is_in_vm(address addr) {
> 1481       assert(false, "Can't find jvm module.");
> 
> I concur that not knowing the JVM base address would be bad.
> I will change the above "assert" calls into "guarantee" calls
> if other reviewers do not object.
> 
> 
> 
>> 1795,1803:
>> else is redundant
> 
> Agreed. My personal preference would be something like:
> 
>   if (cond1) {
>     <some work>
>     return true;
>   }
> 
>   if (cond2) {
>     <some other work>
>     return true;
>   }
> 
>   <work to prepare for false return>
>   return false;
> 
> but I try not to change existing style unless I have to
> make other logic changes to the line. In this case, I'm
> just changing from an implied boolean to '!= 0' so that
> doesn't really meet my criteria.
> 
> The indenting is also wrong in this function and I'm
> trying really hard to not fix... Since this will likely
> get back ported to HSX-24, I shouldn't... must resist...
> 
> Again, thanks for the review!
> 
> Dan
> 
> 
>>
>> -Dmitry
>>
>>
>> On 2013-06-22 02:44, 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.