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

[David Holmes]

LGTM!

Thanks,
David

On 1/05/2020 2:15 am, 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
> 
>