RFR(XXS): 8224793: os::die() does not honor CreateCoredumpOnCrash option

[Daniel D. Daugherty]

David,

Thanks for the fast review! I hoped I would be hearing from you on
this thread :-)


On 5/28/19 11:10 PM, David Holmes wrote:
> On 29/05/2019 11:03 am, Kim Barrett wrote:
>>> On May 28, 2019, at 8:27 PM, Kim Barrett <kim.barrett at oracle.com> 
>>> wrote:
>>> Calling os::exit here seems wrong.  We may have already tried calling
>>> os::exit and gotten here because of problems therein.  And the doc
>>> comment for os::die says "no exit hook, no abort hook, no cleanup."
>
> Yes calling ::exit is wrong.

Agreed (as I replied to Kim). I'm going to look at calling SIGKILL.


>
>>>
>>> SIGABRT (signaled by ::abort) has a default action of Core, which is
>>> the reason for the current behavior.  (And ::abort will invoke that
>>> action if the installed action returns rather than terminating the
>>> process.)  If no core is wanted, SIGKILL has a default action of
>>> Terminate, and that behavior can't be replaced.  So issuing a SIGKILL
>>> when -CreateCoredumpOnCrash seems like it should get the desired
>>> behavior.
>>>
>>> These various implementations of os::die() look like they could be
>>> merged into os_posix.cpp.
>>>
>>
>> And looking at this some more, I think I agree with David's comment in
>> the CR, that os::die *should* always dump core.  It gets called when
>> things have gone horribly wrong, and a core dump might be the only way
>> to understand what went wrong.
>
> Yes - to me "CreateCoredumpOnCrash" only applies to a "crash" that is 
> the result of the VM encountering a (first level) fatal error. If we 
> get secondary crashes that lead to die() then we're in unexpected 
> territory and should dump core by default.

Please see my response to Kim's second set of the comments.
In short: I favor honoring what the test author asked for.


>
>> Maybe there are paths to os::die that ought to be calling os::abort
>> instead?
>
> +1 - maybe something special to handle the crash handler test case?

The '-XX:-CreateCoredumpOnCrash' option is already available to generally
handle what a test author wants.


>
>> I think os::abort probably should not be calling ::exit either, but
>> should be raising SIGKILL if no core dump is requested. os::exit is
>> the path to exit functions and the like.
>
> Yes that is odd. I was going to say people may be relying on it 
> calling atexit functions, but the fact it calls abort else exit 
> depending on the need to dump core suggests no-one could be relying on 
> that.

Issues with os::abort() isn't the bug that I'm trying to solve today
so I'll leave that one alone (for now).

Thanks again for the quick review!

Dan


>
> Thanks,
> David
> -----
>
>> It's unfortunate that the Linux version of os::abort needs to do
>> something special for DumpPrivateMappingsInCore; without that, we
>> could have a merged posix version of os::abort.
>