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

[gerard ziemski]

hi David,

Thank you very much for your review!

> Hi Gerard,
>
> On 21/04/2020 4:33 am, gerard ziemski wrote:
>> hi all,
>>
>> Please review this fix, where we use the knowledge available to us, like whether the ulimit is set, before deciding whether to tell the user in hs_err crash log that we dumped the core file, to match what actually happens.
>>
>> Before this fix, we often assumed that the core file would be created, when in actuality that might not be the case.
> General approach seems fine but some comments below ...
>
>> I also extend an existing test to go with this fix.
>>
>> bug:https://bugs.openjdk.java.net/browse/JDK-8237777
>> webrev:http://cr.openjdk.java.net/~gziemski/8237777_rev1
>> tests:  tested with Mach5 tier-hs1,2,3,4,5
> I'm not quite following all of the changes in relation to what gets printed:
>
> src/hotspot/os/linux/os_linux.cpp
>
> - "\"%s\" (or dumping to %s/core.%d)",
> - &core_pattern[1], p, current_process_id());
> + "\"%s\", or dumping to %s/core",
> + &core_pattern[1], p);
>
> why did you change the pattern? I get core files of the form core.<pid> on my linux box.

I removed it because I thought it was better to be conservative and print the core file name at its minimum - on my Ubuntu OS, for example, the PID is not added to the filename and the OS only uses "core".

I added a comment explaining the assumption here.


> src/hotspot/os/posix/os_posix.cpp
>
> Why did you move the ifdef linux case? What if I don't have infinite stack limit?

That was one of the main issues here. We would print the message about the core file before checking the getrlimit(RLIMIT_CORE), so it would get printed even for the "case 0 stack limit", when no core file gets created.

Having said that, I do not like that message to begin with for 2 reasons:

#1 it's awkwardly phrased, i.e. "Core dump will be written. Default location: Core dumps may be processed with "/usr/share/apport/apport %p %s %c %d %P %E", or dumping to /home/gerard/Desktop/core.13857"

#2 on Ubuntu "apport" (which is the program that processes the core dump) is not enabled by default, so the message is noisy.

Not having that message in the "case default" is therefore not a big deal, however, for consistency I can either add it to both or remove it from both cases. Or  leave it as in my fix.

I have filed JDK-8243196 to address related issues next.


> The printing logic was examined in detail under JDK-8059586: hs_err report should treat redirected core pattern:
>
> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/30ed7423ae23

Thank you for the link.


> ---
>
> src/hotspot/share/utilities/vmError.cpp
>
> Typo: core_dump_ppossible (double p)

Fixed.


> ---
>
> test/hotspot/jtreg/runtime/ErrorHandling/CreateCoredumpOnCrash.java
>
> ! public static void main(String[] args) throws Exception, Throwable {
>
> Exception is redundant now you have added Throwable. But if you change runTest() to throws Exception then you don't need Throwable. But the overload of runTest() seems a bit awkward. I'd just add the new tests directly to main.

How about if I keep the new tests in a separate method, to keep the new code nicely organized, but rename the overloaded method from simply "runTests()" to "checkFix8237777()" or some other suitable name?


new webrev:http://cr.openjdk.java.net/~gziemski/8237777_rev2
testing: in progress...


cheers