RFR: 8324683: Unify AttachListener code for Posix platforms [v4]

[Magnus Ihse Bursie]

On Mon, 15 Apr 2024 13:19:17 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi folks,
>> 
>> This PR aims to unify a lot of the shared code between ```attachListener_linux.cpp``` and ```attachListener_bsd.cpp```. 
>> 
>> Some key points:
>> 
>> 1. AIX: 
>> 
>> ```attachListener_aix.cpp``` has some key differences from linux/bsd, so I have reduced the scope of the issue to linux and bsd only. As a result, there's an ifndef AIX statement wrapping the whole file. Unifying AIX could be a separate issue.
>> 
>> 2. Peer credentials: 
>> 
>> In ```AttachListener::dequeue```, bsd and linux use different APIs to obtain peer credentials. Linux uses ```getsockopt```, while bsd uses ```getpeerid```. 
>> 
>> Bsd doesn't have [SO_PEERCRED ](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getsockopt.2.html). While Linux has ```getpeerid``` implemented on top of ```getsockopt```, it comes in the [bsd compat lib](https://cgit.freedesktop.org/libbsd/tree/src/getpeereid.c). 
>> 
>> To avoid adding further dependencies, I have kept both API calls separated by an ifdef macro. 
>> 
>> Additionally, there are some slight differences in the logging messages in this area for bsd and linux. It's possible to coalesce them should this make more sense. 
>> 
>> 3. ```AttachListener::is_init_trigger()```
>> 
>> Please refer to [mailing list discussion](https://mail.openjdk.org/pipermail/hotspot-runtime-dev/2024-March/068413.html) for details. 
>> 
>> Per the discussion, I tried removing the check for ".attach_pid" in the current working directory (more closely resembling what the bsd code does). However, I noted this caused many failure across linux hotspot tests. See GHA job with failures [here](https://github.com/SoniaZaldana/jdk/actions/runs/8349574000). 
>> 
>> Therefore, I opted for the Linux approach in this PR. This means we do the check in the current working directory across bsd and linux, although this has been removed from bsd before. This seemingly doesn't break any bsd tests.
>> 
>> 5. Minimal JVM Variant: 
>> 
>> I came across an error in the minimal JVM build:
>> ` error: invalid use of incomplete type ‘class AttachOperation’
>>    80 | class PosixAttachOperation : public AttachOperation {`
>> 
>> After a bit of digging, I found that we only get complete definition of AttachOperation inside the directive ```#IF INCLUDE_SERVICES``` [here](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/services/attachListener.hpp#L138 ). 
>> 
>> I added the same directive for the ```attachListener_posix.cpp``` file but I’m not entirel...
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Changing name from posix to nix and updating endif comment

My understanding is that is in the same spot as the "unix" files in the rest of the JDK -- it basically means "not windows". When building on linux, aix or macos, the "unix/posix" files are included. (Also, when building on solaris and bsd on the downstream ports.)

Over the years, I have learned that selecting parts of the code to be used in different builds is not an exact science. It is more of an art form, trying to find the right set of "selectors" to get the right set of code. 

In one extreme, there is the "capability" based approach that the GNU automake/autoconf uses, where you have a myriad of "HAS_xxx" tests. This tends to be way too fine-grained and just be an annoyance when working with, since in practice stuff comes in neat bundles (like "macos") which imply a whole bunch of capabilities.

So the compromise we've kind of settled on is a combination of file include/exclude filters and ifdefs. The art form then is to find the right balance. If you try to group too much together, you end up with a single "generic" file that just contains a lot of ifdefs. And if you split up too much, you end up with myriads of small files, and in the worst case, a lot of code duplication, or code that is split out in functions not because it makes sense from the code's point of view, but because there were implementation differences.

And then you can continue down this line. All "linux":es are not the same. Stuff change between different versions of "macos". We have used ifdefs to separate these concerns, since there is no mechanism to select files based on anything else than os (or cpu). Luckily, such differences also tend to be small.

So, after all this rambling. My point is that "posix" (or "unix") does not really imply anything else than "this is a suitable group of files that we need to have included/excluded on certain platforms". And if that is so, it is good if we use a name that does not imply anything else. The problem with "posix" is that it sounds like the code there should be POSIX compliant. But this is not the case, nor is it even something we should to strive for. In contrast, "unix" is more vague and signals that whatever is included in any particular unix version is okay here, regardless of if it is POSIX or not.

But even if everyone agreed with me that "unix" is a better name than "posix" in this regard, it is not obvious that it is worth renaming the files and directories in hotspot at this time. Just as long as we all keep in mind what it "really" means.

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

PR Comment: https://git.openjdk.org/jdk/pull/18283#issuecomment-2071914245