RFR: 8253742: POSIX signal code cleanup [v5]

[Gerard Ziemski]

On Mon, 9 Nov 2020 03:14:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Thomas' feedback - cleanup ucontext_get_pc/ucontext_set_pc
>
> Latest changes all look good.
> 
> Only open issue is the include, or not, of signal.h.
> 
> Thanks,
> David

> _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. 

Just trying to learn: why do we **need** them? We only included them in the APIs here, but we don't actually **use** them otherwise.

And if we need to include `<signal.h>` then don't we also need to include the headers for `outputStream`, `Thread` and `OSThread`? All of these types are used to define the APIs in this header file and are used in the same capacity.


> 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.

Forward declaration was a new concept to me, so I had to look it up and although it was easy enough to do it for the internal c++ classes, I struggled with the forward declaring of `struct sigset_t`, but I thought I found it.

Should I also drop the forward declaration for `outputStream`, `Thread`, `OSThread`?

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

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