Check core dumping is enabled when set CrashOnOutOfMemoryError

[David Holmes]

On 14/03/2016 6:16 PM, KUBOTA Yuji wrote:
> Hi David,
>
> Sorry, please let discussion a bit more.
>
> 2016-03-14 9:25 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>> First I'm not convinced that you necessarily want to enable core dumps just
>> because you want to crash on OOM.
>>
>> Second I think anyone setting CrashOnOutOfMemoryError can easily remember to
>> set CreateCoredumpOnCrash if they want it.
>
> I think most users do not pay attention about core dumping because
>   * CreateCoredumpOnCrash is enabled by default (jdk9/hs-rt).
>   * Core images not necessarily needed (except crash).
> So we should warn if core dumping is disabled and
> CreateCoredumpOnCrash is enabled, but continue to launch jvm.

What do you mean by "core dumping is disabled"? What is it you expect to 
be checked?

CreateCoredumpOnCrash (formerly CreateMinidumpOnCrash) was primarily a 
windows flag to disable mini-dumps by default on Windows client builds. 
Otherwise we expect the VM to always produce a core dump on crash.

> However, the user who set CrashOnOutOfMemoryError need core images
> clearly, so I think jvm should return error if core dumping is
> disabled.

I disagree. The user who sets CrashOnOutOfMemoryError may also be 
setting ShowMessageBoxOnError so they can attach a debugger or run other 
tools.

The user who wants a core dump can enable core dumps explicitly if needed.

> For example, I want to use CrashOnOOME instead of HeapDumpOnOOME,
> because we can exit jvm definitely and get heapdump and other helpful
> information by serviceability tools.
>
> In conclusion, I have two proposals for avoiding the lack of information.
> 1. Launch jvm with warn message if CreateCoredumpOnCrash is enabled
> but core dumping is disabled.

Again clarify what you mean by this? Given CreateCoredumpOnCrash is 
usually on this would produce warning messages in many execution 
contexts where core dumps are disable (ie ulimit -c 0).  That would be a 
bad thing.

> 2. 1 + Quit with error message if CrashOnOutOfMemoryError is enabled
> but core dumping is disabled.
> I think I should implement 2 in arguments.cpp.
>
> Could you share me your thought?

I don't agree that anything needs to be modified here - sorry.

Thanks,
David
-----


> 2016-03-14 14:59 GMT+09:00 KUBOTA Yuji <kubota.yuji at gmail.com>:
>> Thanks David for looking and comments,
>>
>> 2016-03-14 9:25 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>>> First I'm not convinced that you necessarily want to enable core dumps just
>>> because you want to crash on OOM.
>>
>> Yes, that's right. I just want to avoid the failed of core dumping
>> when we want it clearly by setting options.
>>
>>> Second I think anyone setting CrashOnOutOfMemoryError can easily remember to
>>> set CreateCoredumpOnCrash if they want it.
>>>
>>> Third if I was going to force this on then I would do it directly in
>>> arguments.cpp, not by adding a constraint function.
>>
>> Thanks for sharing information! I will check the implementation of
>> CreateCoredumpOnCrash and update my patch to use arguments.cpp.
>>
>> Thanks,
>> Yuji
>>
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>
>>>> So I create a patch to check the core dumping, please review it.
>>>> I do not have an account of openjdk, so I need sponsor.
>>>>
>>>> I send to serviceability-dev because JDK-8138745 [1] has been
>>>> reviewed here. If unfit, please let me know.
>>>>
>>>> [1]:Implement ExitOnOutOfMemory and CrashOnOutOfMemory in HotSpot
>>>>        https://bugs.openjdk.java.net/browse/JDK-8138745
>>>>
>>>> diff --git a/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
>>>> b/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
>>>> --- a/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
>>>> +++ b/src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp
>>>> @@ -119,6 +119,17 @@
>>>>      }
>>>>    }
>>>>
>>>> +Flag::Error CrashOnOutOfMemoryErrorFunc(bool value, bool verbose) {
>>>> +  char buffer[O_BUFLEN];
>>>> +  os::check_dump_limit(buffer, sizeof(buffer));
>>>> +  if (!VMError::coredump_status) {
>>>> +    CommandLineError::print(verbose,
>>>> +                            "%s\n", VMError::coredump_message);
>>>> +    return Flag::VIOLATES_CONSTRAINT;
>>>> +  }
>>>> +  return Flag::SUCCESS;
>>>> +}
>>>> +
>>>>    Flag::Error PerfDataSamplingIntervalFunc(intx value, bool verbose) {
>>>>      if ((value % PeriodicTask::interval_gran != 0)) {
>>>>        CommandLineError::print(verbose,
>>>> diff --git a/src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp
>>>> b/src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp
>>>> --- a/src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp
>>>> +++ b/src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp
>>>> @@ -43,6 +43,8 @@
>>>>    Flag::Error BiasedLockingBulkRevokeThresholdFunc(intx value, bool
>>>> verbose);
>>>>    Flag::Error BiasedLockingDecayTimeFunc(intx value, bool verbose);
>>>>
>>>> +Flag::Error CrashOnOutOfMemoryErrorFunc(bool value, bool verbose);
>>>> +
>>>>    Flag::Error PerfDataSamplingIntervalFunc(intx value, bool verbose);
>>>>
>>>>    #endif /* SHARE_VM_RUNTIME_COMMANDLINEFLAGCONSTRAINTSRUNTIME_HPP */
>>>> diff --git a/src/share/vm/runtime/globals.hpp
>>>> b/src/share/vm/runtime/globals.hpp
>>>> --- a/src/share/vm/runtime/globals.hpp
>>>> +++ b/src/share/vm/runtime/globals.hpp
>>>> @@ -1401,6 +1401,7 @@
>>>>      product(bool, CrashOnOutOfMemoryError, false,
>>>> \
>>>>              "JVM aborts, producing an error log and core/mini dump, on the
>>>> "  \
>>>>              "first occurrence of an out-of-memory error")
>>>> \
>>>> +          constraint(CrashOnOutOfMemoryErrorFunc,AfterErgo)
>>>> \
>>>>
>>>> \
>>>>      /* tracing */
>>>> \
>>>>
>>>> \
>>>> diff --git a/src/share/vm/utilities/vmError.hpp
>>>> b/src/share/vm/utilities/vmError.hpp
>>>> --- a/src/share/vm/utilities/vmError.hpp
>>>> +++ b/src/share/vm/utilities/vmError.hpp
>>>> @@ -65,14 +65,6 @@
>>>>      // so use thread id instead of Thread* to identify thread.
>>>>      static volatile intptr_t first_error_tid;
>>>>
>>>> -  // Core dump status, false if we have been unable to write a
>>>> core/minidump for some reason
>>>> -  static bool coredump_status;
>>>> -
>>>> -  // When coredump_status is set to true this will contain the
>>>> name/path to the core/minidump,
>>>> -  // if coredump_status if false, this will (hopefully) contain a
>>>> useful error explaining why
>>>> -  // no core/minidump has been written to disk
>>>> -  static char coredump_message[O_BUFLEN];
>>>> -
>>>>
>>>>      // set signal handlers on Solaris/Linux or the default exception
>>>> filter
>>>>      // on Windows, to handle recursive crashes.
>>>> @@ -111,6 +103,14 @@
>>>>      // Record status of core/minidump
>>>>      static void record_coredump_status(const char* message, bool status);
>>>>
>>>> +  // Core dump status, false if we have been unable to write a
>>>> core/minidump for some reason
>>>> +  static bool coredump_status;
>>>> +
>>>> +  // When coredump_status is set to true this will contain the
>>>> name/path to the core/minidump,
>>>> +  // if coredump_status if false, this will (hopefully) contain a
>>>> useful error explaining why
>>>> +  // no core/minidump has been written to disk
>>>> +  static char coredump_message[O_BUFLEN];
>>>> +
>>>>      // support for VM.info diagnostic command
>>>>      static void print_vm_info(outputStream* st);
>>>>
>>>>
>>>> Thanks,
>>>> Yuji
>>>>
>>>