RFR: 8253742: POSIX signal code cleanup [v5]

[Gerard Ziemski]

On Mon, 9 Nov 2020 20:53:12 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> _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`?
>
>> > > 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.
> 
> You need to include them if you:
> - use the full type, so whoever compiles the header has to know the type size. E.g. if you pass a structure by value.
> - use the type as pointer and do not forward declare it.
> 
> In hotspot, it is standard practice to forward-declare structures from some hotspot utility headers - eg ostream.hpp - to avoid including them. There is no guideline of when to do this, and obviously there is a point at which it is simpler and clearer to just include the header.
> 
> In my personal opinion forward declaration is just a bandaid to work around badly designed headers. Ideally, headers should be small and concise. But hotspot headers are balls of yarn. Pull one in you get a bunch of unrelated others. So people got used to forward declaring some classes instead, things like outputStream. I actually think its a bad practice and in the ideal world we would just include whatever we need.
> 
> Which also means that the benefit of forward-declaring types from system headers is limited. Posix headers are usually well designed, and you are better off just including them. Especially since you need to be careful here: it is not clear what these opaque posix types are actually. Sometimes the standard tells you: "The <signal.h> header shall define the siginfo_t type as a structure.." but sometimes it leaves it open: "sigset_t ... Integer or structure type ...".  
> 
>> 
>> > 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`?
> 
> For hotspot classes, I would leave the forward declarations in and the headers out. Current standard practice. System headers OTOH I would either include here or, somewhat better, in globalDefinitions_gcc.hpp.

> > > 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`?
> > 
> > 
> > For hotspot classes, I would leave the forward declarations in and the headers out. Current standard practice. System headers OTOH I would either include here or, somewhat better, in globalDefinitions_gcc.hpp.
> 
> Ha! It is already in globalDefinitions_gcc.hpp so neither the direct
> include nor the forward declarations are actually needed.

Many thanks Thomas & David for the lesson on the header files!

If I understand it correctly, then we need to add `#include "utilities/globalDefinitions.hpp"` to signals_posix.hpp, correct?

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

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