8352392: AIX: implement attach API v2 and streaming output

[varada1110]

AIX changes for attach API to support arbitrary length arguments and the streaming output support.
serviceability/attach/AttachAPIv2/StreamingOutputTest.java test passes

tier1, tier2 and tier3 testing is successful with fastdebug level

JBS Issue : JDK-8352392

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8352392: AIX: implement attach API v2 and streaming output (Enhancement - P4)

Reviewers

• Alex Menkov (@alexmenkov - Reviewer) Review applies to d22f0ab9
• Martin Doerr (@TheRealMDoerr - Reviewer)
• Joachim Kern (@JoKern65 - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24177/head:pull/24177
$ git checkout pull/24177

Update a local copy of the PR:
$ git checkout pull/24177
$ git pull https://git.openjdk.org/jdk.git pull/24177/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24177

View PR using the GUI difftool:
$ git pr show -t 24177

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24177.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back varadam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@varada1110 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8352392: AIX: implement attach API v2 and streaming output

Reviewed-by: mdoerr, jkern, amenkov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 90 new commits pushed to the master branch:

• 70e3250: 8352419: Test tools/jpackage/share/ErrorTest.java#id0 and #id1 fail
• 296d9d6: 8353345: C2 asserts because maskShiftAmount modifies node without deleting the hash
• 3ceabf0: 8353359: C2: Or(I|L)Node::Ideal is missing AddNode::Ideal call
• b263292: 8353484: JFR: Simplify EventConfiguration
• ffca4f2: 8353264: ZGC: Windows heap unreserving is broken
• f7a94fe: 8352585: Add special case handling for Float16.max/min x86 backend
• 9c5ed23: 8349988: Change cgroup version detection logic to not depend on /proc/cgroups
• 00a038e: 8353341: C2: removal of a Mod[DF]Node crashes when the node is already dead
• bd74922: 8338675: javac shouldn't silently change .jar files on the classpath
• 8d3d1d4: 8353058: [PPC64] Some IR framework tests are failing after JDK-8352595
• ... and 80 more: https://git.openjdk.org/jdk/compare/ddf326b8e6e50403303b410635e4c26d7bf56aaa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@varada1110 The following labels will be automatically applied to this pull request:

• hotspot-runtime
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 03: Full - Incremental (234f6d17)
• 02: Full - Incremental (d22f0ab9)
• 01: Full - Incremental (ab6941f2)
• 00: Full (8efd253f)

[JoKern65]

Hi Varada, it looks to me that with your change you've brought the AIX code closer to the POSIX implementation, but not completely. Could you please explain your changes in detail so I can review the PR? I'm not that familiar with this.

[varada1110]

test/hotspot/jtreg/serviceability/attach/AttachAPIv2/StreamingOutputTest.java

Initial failure
Execution failed: `main' threw exception: java.lang.Exception: VM did not logged expected 'executing command setflag, streaming output: 1'

AIX hasn’t adopted changes to support the new protocol version v2

jdk/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java

Line 114 in 8efd253

if (isAPIv2Enabled()) {

. Additionally aix doesn’t have the changes to support streaming output. The "getversion” command supports an "options" argument which currently takes option "streaming"which allows client to control streaming/buffered output mode.
On aix, this process uses unix sockets for communications like posix had more similar implementation. Is there anything additional changes I should take care of?

#20782 (comment)
#23405 (comment)

[alexmenkov]

The change looks good.
Couple notes:

• need to update copyright year in VirtualMachineImpl.java
• need to remove StreamingOutputTest.java from problem list (revert JDK-8352393)

[varada1110]

Thank you @alexmenkov, I have updated copyright year and reverted the changes from JDK-8352393.

[TheRealMDoerr]

I think this is good. @JoKern65: Do you also want to review it?

[JoKern65]

Hi Varada, I see that you've largely adapted the code to the POSIX version. Essentially, only the shutdown special handling remains. But the AIX special handling that you removed was introduced for some reason.
Do you know why? Does this reasoning no longer apply?
I have no idea and can't judge whether the different semantics have created holes in the code that could reappear under certain circumstances.
Therefore, I would like an explanation from you as to why you can make this change now.

[varada1110]

With the Attach API protocol update to version 2, now support arbitrary-length arguments, and shared code has been added in attachListener.cpp (read_request, write_fully) to handle this across platforms. As a result, the AIX code is no longer required. I also compared the older changes in the POSIX implementation and found them to be the same as AIX

[JoKern65]

Reasonable change.

[varada1110]

Thanks all,
/integrate

[openjdk]

Going to push as commit 41d4a0d.
Since your change was applied there have been 99 commits pushed to the master branch:

• 1c2a553: 8327858: Improve spliterator and forEach for single-element immutable collections
• a449aee: 8350704: Create tests to ensure the failure behavior of core reflection APIs
• 57df89c: 8353684: [BACKOUT] j.u.l.Handler classes create deadlock risk via synchronized publish() method
• ebcb9a8: 8349206: j.u.l.Handler classes create deadlock risk via synchronized publish() method
• d894b78: 8353588: [REDO] DaCapo xalan performance with -XX:+UseObjectMonitorTable
• db08726: 8352966: Opensource Several Font related tests - Batch 2
• 6b7b324: 8351431: Type annotations on new class creation expressions can't be retrieved
• 64b691a: 8271870: G1: Add objArray splitting when scanning object with evacuation failure
• b428cda: 8349686: [s390x] C1: Improve Class.isInstance intrinsic
• 70e3250: 8352419: Test tools/jpackage/share/ErrorTest.java#id0 and #id1 fail
• ... and 89 more: https://git.openjdk.org/jdk/compare/ddf326b8e6e50403303b410635e4c26d7bf56aaa...master

Your commit was automatically rebased without conflicts.

[openjdk]

@varada1110 Pushed as commit 41d4a0d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.