RFR (S): 8237777 "Dumping core ..." is shown despite claiming that "# No core dump will be written."

[Yasumasa Suenaga]

Looks good.

Yasumasa (ysuenaga)


On 2020/05/01 1:15, gerard ziemski wrote:
> 
> 
> On 4/29/20 6:33 PM, David Holmes wrote:
>> On 30/04/2020 2:39 am, gerard ziemski wrote:
>>>
>>>
>>> On 4/24/20 11:44 PM, David Holmes wrote:
>>>> On 25/04/2020 4:02 am, gerard ziemski wrote:
>>>>>
>>>>>
>>>>> On 4/23/20 7:55 PM, David Holmes wrote:
>>>>>
>>>>> [trimmed]
>>>>>
>>>>>> Given that the "Dumping core ..." is only shown in non-product builds I think we are way over-thinking/engineering this. We have:
>>>>>>
>>>>>>   if (dump_core) {
>>>>>>   ...
>>>>>> #ifndef PRODUCT
>>>>>>     ...
>>>>>>     out.print_raw_cr("Dumping core ...");
>>>>>> #endif
>>>>>>     ::abort(); // dump core
>>>>>>   }
>>>>>>
>>>>>> so lets just change one line:
>>>>>>
>>>>>>     out.print_raw_cr("Requesting a core dump if possible ...");
>>>>>>
>>>>>> otherwise you need one boolean to control whether we call abort vs exit, and a second boolean to control what we print.
>>>>>>
>>>>>> Or change that one line to:
>>>>>>
>>>>>>     out.print_raw_cr("Calling ::abort() ...");
>>>>>
>>>>> Making sure we do not mess up the core creation logic is paramount, so I like your suggestion to keep it simple, but perhaps we can take it one small step further and just not bother with that message at all?
>>>>>
>>>>> Maybe we should simply remove the line:
>>>>>
>>>>>     out.print_raw_cr("Dumping core ...");
>>>>>
>>>>> completely? After all, we print lots of core dumping related info anyhow.
>>>>
>>>> We've been printing
>>>>
>>>>   Current thread is xxx
>>>>   Dumping core ...
>>>>
>>>> to the default output stream in non-product builds since day one of this code - literally. Why only non-product and why only for when dump_core is true I have no idea. The whole thing seems somewhat superfluous. I'd suggest either tweaking the second line, or removing it all, rather than just deleting the second line (which gives some additional context to the first).
>>>
>>> I gave it some thought, looked at a few Linux distros, and personally I'd like to see both of those lines to go away. We already print the crashed thread info in the hs_err crash log, even before we get to those 2 lines, so we're not risking loosing the "current thread" info.
>>
>> I believe those 2 lines will always print to the console/tty. It is possible if hs_err log generation fails that only the console output may be seen. But in that case without a hs_err log I don't think those two lines really add anything useful.
>>
>>> bug:https://bugs.openjdk.java.net/browse/JDK-8237777
>>> webrev:http://cr.openjdk.java.net/~gziemski/8237777_rev3
>>
>> The webrev only contains os_bsd.cpp. ??
> 
> Sorry, here is the webrev for all the relevant platforms:
> 
> webrev:http://cr.openjdk.java.net/~gziemski/8237777_rev4
> testing: hs_tier1,2,3,4,5,6,7 in progress...
> 
> I wasn't 100% sure whether to include Solaris here, but I did in the end, though I can skip it if that will mess up the effort to remove Solaris support.
> 
> 
> cheers
> 
>