8342995: Enhance Attach API to support arbitrary length arguments - Linux #22223

alexmenkov · 2024-11-19T00:04:16Z

The fix updates Linux (and server-side of macosx) implementation to support Attach API v2 (shared code and Windows implementation were introduced by #20782)

Testing: tier1,tier2,tier3,tier4,hs-tier5-svc
manually tested backward compatibility (old tools can attach to current VMs, current tools can attach to older VMs) with jdk21 and jdk8.

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-8342995: Enhance Attach API to support arbitrary length arguments - Linux (Sub-task - P3)

Reviewers

Kevin Walls (@kevinjwalls - Reviewer) Review applies to 2ca15a18
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22223

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

Using diff file

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

Using Webrev

Link to Webrev Comment

alexmenkov · 2024-11-19T00:04:33Z

/label add serviceability

bridgekeeper · 2024-11-19T00:04:48Z

👋 Welcome back amenkov! 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 · 2024-11-19T00:05:34Z

@alexmenkov 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:

8342995: Enhance Attach API to support arbitrary length arguments - Linux

Reviewed-by: sspitsyn, kevinw

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 614 new commits pushed to the master branch:

18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
31c3b19: 8346174: UMAX/UMIN are missing from XXXVector::reductionOperations
c75b1d4: 8346082: Output JVMTI agent information in hserr files
51662c2: 8346232: Remove leftovers of the jar --normalize feature
d335941: 8345678: compute_modifiers should not be in create_mirror
57adf64: 8346261: Cleanup in JDP tests
... and 604 more: https://git.openjdk.org/jdk/compare/ec148c136555899c90f773b2904baf459efac3af...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 · 2024-11-19T00:06:20Z

@alexmenkov
The serviceability label was successfully added.

mlbridge · 2024-11-19T00:09:12Z

Webrevs

kevinjwalls

Looks good I think. (These are kind of hard to read, or hard to appreciate as a whole!)

VirtualMachineImpl.java and attachListener_PLATFORM.cpp have much in common between platforms, but maybe creating new common base classes is an interesting refactor work with not that much payoff. Plus there are AIX versions that some of us don't build and test (which I think you mentioned elsewhere).

On Windows, attachListener_windows.cpp operations have: assert(opened(), "must be");
...I was wondering if we don't need a similar assert in in src/hotspot/os/posix/attachListener_posix.cpp
But probably the different usage pattern explains that, i.e.
Windows' PipeChannel needs the open() method specifically called, but for Posix it creates the socket on construction.

(so this is fine, just some observations)

alexmenkov · 2024-12-09T22:40:10Z

VirtualMachineImpl.java and attachListener_PLATFORM.cpp have much in common between platforms, but maybe creating new common base classes is an interesting refactor work with not that much payoff.

Yes, there a lot of common code between platforms and it can be refactored, but it would make the fix much bigger and harder to review.

Plus there are AIX versions that some of us don't build and test (which I think you mentioned elsewhere).

Right. Most of the AIX code is copy of the posix/linux/macosx implementation, but I have no environment to build/test it, so I changed shared code to not break AIX build.

On Windows, attachListener_windows.cpp operations have: assert(opened(), "must be"); ...I was wondering if we don't need a similar assert in in src/hotspot/os/posix/attachListener_posix.cpp But probably the different usage pattern explains that, i.e. Windows' PipeChannel needs the open() method specifically called, but for Posix it creates the socket on construction.

Right. The assert can be added to posix implementation, but it doesn't make much sense as SocketChannel does not opens the socket, but get it from the caller

kevinjwalls

Looks good, thanks, a couple of other notes here in case it helps anyone.

AttachOperation::write_reply(ReplyWriter * writer, jint result, const char* message, int message_len)
is this only called by the write_reply method that passes message_len as strlen(stream) ?
The new check for len<0 might not really be useful but does no harm.

The existing "char msg[32]" is a buffer used to write the numeric jint result, not the message as the name might suggest.

HotSpotVirtualMachine.java already has writeCommand() that handles VERSION_2, so the Linux VirtualMachineImpl can just call it. Also writeString is already there in HotSpotVirtualMachine, so removal from Linux is a good cleanup.

I'm sure we all want to find time for the future conversation about actually streaming the output rather than passing a single buffer around. 8-)

alexmenkov · 2024-12-10T19:21:52Z

Looks good, thanks, a couple of other notes here in case it helps anyone.

AttachOperation::write_reply(ReplyWriter * writer, jint result, const char* message, int message_len) is this only called by the write_reply method that passes message_len as strlen(stream) ? The new check for len<0 might not really be useful but does no harm.

There are couple calls from AttachOperation::read_request that passes const char* (message_len is -1 as the default value).
I.e we need 2 versions of write_reply - one for bufferedStream and another for const char*.

The existing "char msg[32]" is a buffer used to write the numeric jint result, not the message as the name might suggest.

This is old code (similar to deleted from PosixAttachOperation::complete).
Do you want to rename msg to buf?

I'm sure we all want to find time for the future conversation about actually streaming the output rather than passing a single buffer around. 8-)

I'm open for it :)

kevinjwalls · 2024-12-10T20:41:04Z

Do you want to rename msg to buf?
Don't worry about going back and renaming it just for me 8-)

src/hotspot/share/services/attachListener.hpp

src/hotspot/os/posix/attachListener_posix.cpp

alexmenkov · 2024-12-11T21:52:34Z

Do you want to rename msg to buf?
Don't worry about going back and renaming it just for me 8-)

Did it with the last update

sspitsyn

The fix looks good modulo a couple of nits.

sspitsyn · 2024-12-13T11:50:52Z

src/hotspot/share/services/attachListener.cpp

+    // We know the whole request does not exceed buffer_size,
+    // for v1 also name/arguments should not exceed name_length_max/arg_length_max.
+    if (strlen(name()) > AttachOperation::name_length_max) {
+      log_error(attach)("Failed to read request: name is too long");


Nit: I'd suggest to be more specific: "Failed to read request: attach operation name is too long"

There is "attach" in the log prefix, so I made it "Failed to read request: operation name is too long"
And updated similar logging for arguments several lines below

sspitsyn · 2024-12-13T11:52:09Z

src/hotspot/share/services/attachListener.hpp

@@ -231,6 +235,8 @@ class AttachOperation: public CHeapObj<mtServiceability> {
  // complete operation by sending result code and any result data to the client
  virtual void complete(jint result, bufferedStream* result_stream) = 0;

+  class ReplyWriter; //forward declaration


Nit: The comment should start with a space.

alexmenkov · 2024-12-17T18:48:39Z

Kevin, Serguei, thank you for the review
/integrate

openjdk · 2024-12-17T18:50:03Z

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

4f44cf6: 8341481: [perf] vframeStreamCommon constructor may be optimized
390b205: 8346048: test/lib/containers/docker/DockerRunOptions.java uses addJavaOpts() from ctor
03821d9: 8346195: Fix static initialization problem in GDIHashtable
a5503fb: 8346432: java.lang.foreign.Linker comment typo
fbd76ca: 8337016: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java gets Metaspace OOM
baeb3d9: 8346304: SA doesn't need a copy of getModifierFlags
99af595: 8345942: Separate source output from class output when building microbenchmarks
8a64595: 8346282: [JVMCI] Add failure reason support to UnresolvedJava/Type/Method/Field
725079b: 8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException
5e25c48: 8346289: Confusing phrasing in IR Framework README / User-defined Regexes
... and 618 more: https://git.openjdk.org/jdk/compare/ec148c136555899c90f773b2904baf459efac3af...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-17T18:50:14Z

@alexmenkov Pushed as commit dc71e8c.

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

attach_v2_linux

2ca15a1

openjdk bot added rfr Pull request is ready for review serviceability serviceability-dev@openjdk.org labels Nov 19, 2024

kevinjwalls reviewed Dec 9, 2024

View reviewed changes

kevinjwalls approved these changes Dec 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 10, 2024

sspitsyn reviewed Dec 11, 2024

View reviewed changes

src/hotspot/share/services/attachListener.hpp Outdated Show resolved Hide resolved

src/hotspot/os/posix/attachListener_posix.cpp Outdated Show resolved Hide resolved

feedback

ae36427

openjdk bot removed the ready Pull request is ready to be integrated label Dec 11, 2024

updated comment

27bb3c4

sspitsyn approved these changes Dec 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 13, 2024

feedback

24ab756

openjdk bot removed the ready Pull request is ready to be integrated label Dec 13, 2024

sspitsyn approved these changes Dec 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 17, 2024

openjdk bot closed this Dec 17, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 17, 2024

openjdk bot removed the rfr Pull request is ready for review label Dec 17, 2024

alexmenkov deleted the attach_v2_linux branch December 17, 2024 21:02

alexmenkov mentioned this pull request Jan 3, 2025

8342996: Enhance Attach API to support arbitrary length arguments - OSX #22907

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8342995: Enhance Attach API to support arbitrary length arguments - Linux #22223

8342995: Enhance Attach API to support arbitrary length arguments - Linux #22223

alexmenkov commented Nov 19, 2024 •

edited by openjdk bot

Loading

alexmenkov commented Nov 19, 2024

bridgekeeper bot commented Nov 19, 2024

openjdk bot commented Nov 19, 2024 •

edited

Loading

openjdk bot commented Nov 19, 2024

mlbridge bot commented Nov 19, 2024 •

edited

Loading

kevinjwalls left a comment

alexmenkov commented Dec 9, 2024

kevinjwalls left a comment

alexmenkov commented Dec 10, 2024

kevinjwalls commented Dec 10, 2024

alexmenkov commented Dec 11, 2024

sspitsyn left a comment

sspitsyn Dec 13, 2024

alexmenkov Dec 13, 2024

sspitsyn Dec 13, 2024

alexmenkov Dec 13, 2024

alexmenkov commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

8342995: Enhance Attach API to support arbitrary length arguments - Linux #22223

8342995: Enhance Attach API to support arbitrary length arguments - Linux #22223

Conversation

alexmenkov commented Nov 19, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

alexmenkov commented Nov 19, 2024

bridgekeeper bot commented Nov 19, 2024

openjdk bot commented Nov 19, 2024 • edited Loading

openjdk bot commented Nov 19, 2024

mlbridge bot commented Nov 19, 2024 • edited Loading

Webrevs

kevinjwalls left a comment

Choose a reason for hiding this comment

alexmenkov commented Dec 9, 2024

kevinjwalls left a comment

Choose a reason for hiding this comment

alexmenkov commented Dec 10, 2024

kevinjwalls commented Dec 10, 2024

alexmenkov commented Dec 11, 2024

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn Dec 13, 2024

Choose a reason for hiding this comment

alexmenkov Dec 13, 2024

Choose a reason for hiding this comment

sspitsyn Dec 13, 2024

Choose a reason for hiding this comment

alexmenkov Dec 13, 2024

Choose a reason for hiding this comment

alexmenkov commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

alexmenkov commented Nov 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 19, 2024 •

edited

Loading

mlbridge bot commented Nov 19, 2024 •

edited

Loading