RFR(M): 8219584: Try to dump error file by thread which causes safepoint timeout

[David Holmes]

Hi Martin,

I like the general functionality but not the code placement. :) It';s 
easiest to build on Thomas's response below ...

On 28/02/2019 12:57 am, Thomas Stüfe wrote:
> Hi Martin,
> 
> I like this. I have minor points, most of them a matter of taste:
> 
> ¹) I actually do not like much having os::signal_thread(int sig). 
> Signals are a Posix concept, there is nothing on windows like it. That 

Perhaps not thread-directed signals but still:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2017

I don't have any concern about this being an os function even if the 
Windows version is basically a no-op ("return false;"). However it 
should not be defined in os.cpp with a bunch of ifdefs! That's why we 
have the os_<os>.cpp files. There should be one version in os_posix.cpp 
(may as well keep the Solaris ifdefs there) and one in os_windows.cpp.

> is why we still need +#ifndef _WINDOWS in safepoint.cpp, even though 
> os::signal_thread() is available on windows too.

I don't see why any ifdef is needed in safepoint.cpp, on windows we'll 
break out of the loop on the first iteration. We shouldn't care about 
overhead here (and maybe someday windows signal_thread will do something).

<snip>

> 
> 2) in vmError.cpp, you want to have the signal info more prominently, 
> which I like. But I would just call print_siginfo() there. It is defined 
> to print a single line without /cr, so it should fit nicely into the 
> "#.." section.
> If you do not like the idea, you would have to not only handle TKILL but 
> also SI_KILL and SI_QUEUE (I think), duplicating the work in 
> print_siginfo. Note that TKILL is, I think, only available on Linux.

I think I agree with Thomas. I really do not like the ifdefs added to 
vmError.cpp. This should be printed as part of the signal info and 
handled in the appropriate platform code.

Other comments:

In signal_thread why are we using Event::log? I'm not familiar with it.

In the test:

   52             // Long running loop without safepoint.
   53             for (int y = 1; y < Integer.MAX_VALUE; ++y) {
   54                 if (y % x == 0) ++sum;
   55             }

doesn't the safepoint polling depend on the value of 
UseCountedLoopSafepoints? I'd want a compiler person to check that this 
test is reliably going to check what you want it to check.

Thanks,
David
-----