RFR: 8253742: POSIX signal code cleanup [v3]

[Thomas Stuefe]

On Wed, 4 Nov 2020 05:06:14 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi Gerard,
>> 
>> Overall looking good. Some changes still to be finalized e.g ucontext_t related functions in os.hpp.
>> 
>> I flagged some os functions that are implemented in os_foo.cpp but which just call the Posix helper, which can be deleted from os_foo.cpp and simply added to os_posix.cpp. That can't be a further cleanup RFE if you want to limit changes in this PR.
>> 
>> A few minor nits below.
>> 
>> Thanks,
>> David
>
>> The snapshot of JDK that I'm using in my PR does not build on Windows. Do you have any suggestion how I can safely update to the latest JDK without messing up my PR?
> 
> Hi Gerard,
> 
> merging master would not help you with the build error. As I said, it complains about ucontext_t. 
> 
> As for your question, just do a 
> 
> git checkout yourbranch
> git merge master
> 
> if you get conflicts, you'll need to resolve them, but this is the way to go without invalidating old commits.

> _Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
> 
> On 7/11/2020 2:40 am, Gerard Ziemski wrote:
> 
> > On Wed, 4 Nov 2020 04:22:05 GMT, David Holmes <dholmes at openjdk.org> wrote:
> > > > Gerard Ziemski has updated the pull request incrementally with two additional commits since the last revision:
> > > > - use ifdef(SIGDANGER) and ifdef(SIGTRAP)
> > > > - revert unblock_program_error_signals change
> > > 
> > > 
> > > src/hotspot/os/posix/signals_posix.hpp line 33:
> > > > 31:
> > > > 32: typedef siginfo_t siginfo_t;
> > > > 33: typedef sigset_t sigset_t;
> > > 
> > > 
> > > I don't see why this is needed/wanted. We can include signal.h without a problem.
> > > I'm not even sure what these typedefs means ??
> > 
> > 
> > Coleen asked me to remove <signal.h> from the signals_posix.hpp, so those are forward declarations for the signal types we use.
> > I thought it was a reasonable request to minimize the number of headers. I saw some efforts in the past to cleanup header files, which is supposed to help with build times, so every little bit helps.
> 
> The only reason we would care is if signals_posix.hpp were included in
> many other handers/files and that should not be the case. This looks
> completely bogus to me as we need the types from signal.h in this header
> file. What do those typedefs even mean? I would expect a forward
> declaration to be of the form:
> 
> struct siginfo_t;
> 
> but you don't know what type sigset_t (could be integer or struct)
> actually is so you can't forward declare it that way.
> 

Oh you are right. Posix guarantees that siginfo_t and ucontext_t are structures; sigset_t can be either a structure or an integer type. Missed that.

I'd just either leave out the header. Or even leave it in. Do we have a policy like "all system headers only go into one central place, e.g. globalDefinitions"? Because if not, Gerard's original code would be more correct (where he included signal.h)

Just my 5 cent. I'll keep out of this discussion for now. Any form is fine for me.

Cheers Thomas

> David
> -----

-------------

PR: https://git.openjdk.java.net/jdk/pull/636