8339289: Enhance Attach API to support arbitrary length arguments - Windows #20782

alexmenkov · 2024-08-30T00:07:50Z

The fix improves Attch API protocol and implements updated protocol on windows; shared code is ready to implement updated protocol support on other platforms.
More detailed explanations on the 1st comment.

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) on Windows with jdk21u and jdk8u.

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

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) Review applies to 86bd1cd2
Kevin Walls (@kevinjwalls - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20782

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

Using diff file

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

Webrev

Link to Webrev Comment

alexmenkov · 2024-08-30T00:08:06Z

Currently Attach API has the following restrictions:

attach operation must have exactly 3 arguments; The value cannot be changed due backward compatibility - tools from previous release should be able to attach to current java processes, current tool should be able to work with older java processes;
each argument is resticted by 1024 symbols; this may be not enough as some arguments contain file paths.

DCmd framework supports any number of command arguments, but encodes all arguments as a single attach operation arguement, so total length of all arguments are restricted by 1024 symbols.

Attach API changes:

version of the protocol is bumped to 2; attach operation request v2 contains length of the request, so request can contain any number of argument of arbitrary length (for security reason request length is restricted by 256K);
for backward compatibility both client and server sides support both v1 and v2; if v2 is not supported by tool or target JVM, v1 is used for communication;
new attach operation is implemented: "getVersion"; old VMs report "Operation not recognized", new VMs report supported version;
for testing purposes "jdk.attach.compat" system property is introduced to disable v2 protocol;

Windows implementation:
Windows implementation is different from other platforms (other platforms use unix sockets for communications).
On Windows client enqueues operation by copying request parameters and executing remote thread in the target VM process which calls special exported function. Operation results are returned by using pipe (1-directional), created by the client;

to enqueue operation v2 new exported function has been added; only pipe name is passed to the target VM for v2 requests;
all operation parameters are passed through pipe, operation results are returned through the same pipe (it becomes 2-directional).

bridgekeeper · 2024-08-30T00:08:15Z

👋 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-08-30T00:09:05Z

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

8339289: Enhance Attach API to support arbitrary length arguments - Windows

Reviewed-by: kevinw, sspitsyn

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

d1540e2: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
7af46a6: 8340554: Improve MessageFormat readObject checks
7d5eefa: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
... and 758 more: https://git.openjdk.org/jdk/compare/b711c41d442fc369a84745c0203db638e0b7e671...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-08-30T00:09:31Z

⚠️ @alexmenkov This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2024-08-30T00:09:39Z

@alexmenkov 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 · 2024-08-30T00:12:30Z

Webrevs

dcubed-ojdk · 2024-08-30T18:18:03Z

Thanks for putting the details in a separate comment. That will make the emails
that get sent for this PR much shorter.

You use the word symbols, e.g., each argument is resticted by 1024 symbols.
Do you really mean characters or bytes?

dcubed-ojdk · 2024-08-30T18:18:29Z

Also thanks for including all the testing info!

alexmenkov · 2024-08-30T20:03:58Z

You use the word symbols, e.g., each argument is resticted by 1024 symbols. Do you really mean characters or bytes?

Bytes. The code used char buffers with fixed size.

alexmenkov · 2024-09-23T23:31:12Z

Looking for reviewers for the fix

sspitsyn

This is nice fix, thank you for doing this!
I still need one more pass on the Windows AttachListener part and hope to complete it early next week.

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

src/hotspot/share/services/attachListener.cpp

renamed JVM_EnqueueOperation2 to JVM_EnqueueOperation_v2 for consistency (per Serguei request)

src/hotspot/os/windows/attachListener_windows.cpp

sspitsyn

I've posted some nits and one questions.
Otherwise, this change looks good to me.
Thank you for the updates.

sspitsyn

Thank you for the updates. Looks good.

kevinjwalls · 2024-10-22T10:12:10Z

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

+
+    public static void main(String[] args) throws Exception {
+        // if the test (client part) in the "compat" mode
+        boolean clientCompat = "true".equals(System.getProperty("jdk.attach.compat"));


Boolean.getBoolean("jdk.attach.compat") ignores case for us as well.
But more generally, are we happy with "jdk.attach.compat" as a boolean flag, or would "jdk.attach.version" be better?
(which is mainly asking if we think there might ever be another version!)

I thought about making the property numeric, but that adds complexity in the logic and I don't think we will need another version soon. Anyway it's internal property for testing only, so we can change it any time.

src/hotspot/share/services/attachListener.hpp

src/hotspot/share/services/attachListener.cpp

kevinjwalls · 2024-10-24T21:29:37Z

Thanks for updating, looks good. I think it's clearer now that this is not just a Windows-specific fix, but will be an enhancement for all platforms in the long term. Likely argument length is more of a limitation than number of arguments.

Looking back at JDK-8215622: Add dump to file support for jmap –histo
..and that was extending the "inspectheap" attach command, but it should have been using a DiagnosticCommand invoked by jcmd. We may not say it clearly, but the attach api commands are a few basic fundamentals, and most of what we want to implement should be implemented in a DiagnosticCommand.

alexmenkov · 2024-10-24T23:08:58Z

Thank you for review Serguei and Kevin
I'm going to rebase the change and run sanity tests before integration

alexmenkov · 2024-10-25T18:07:19Z

/integrate

openjdk · 2024-10-25T18:08:22Z

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

ff165f9: 8342934: TYPE_USE annotations printed with error causing "," in toString output
0853aee: 8338426: Test java/nio/channels/Selector/WakeupNow.java failed
c202a2f: 8295269: G1: Improve slow startup due to predictor initialization
5cbd578: 8342930: New tests from JDK-8335912 are failing
1e35da8: 8343063: RISC-V: remove redundant reg copy in generate_resolve_blob
4f8f395: 8343060: RISC-V: enable TestFloat16VectorConvChain for riscv
a9eb50a: 8342953: RISC-V: Fix definition of RISCV_HWPROBE_EXT_ZVFHMIN
94317db: 8342884: RISC-V: verify float <--> float16 conversion
3c5db12: 8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout
4635351: 8342939: Building ZGC without compiler2 fails
... and 772 more: https://git.openjdk.org/jdk/compare/b711c41d442fc369a84745c0203db638e0b7e671...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-25T18:08:29Z

@alexmenkov Pushed as commit 36d7173.

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

alexmenkov added 3 commits August 29, 2024 16:03

attach api v2

3bd09c0

attach api v2

918500f

Merge branch 'attach_v2' of github.com:alexmenkov/jdk into attach_v2

fd52c13

openjdk bot added the rfr Pull request is ready for review label Aug 30, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org labels Aug 30, 2024

sspitsyn reviewed Oct 12, 2024

View reviewed changes

src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java Outdated Show resolved Hide resolved

src/hotspot/share/services/attachListener.cpp Show resolved Hide resolved

alexmenkov added 2 commits October 14, 2024 13:28

feedback

2e904c5

renamed JVM_EnqueueOperation2

1fed4ea

renamed JVM_EnqueueOperation2 to JVM_EnqueueOperation_v2 for consistency (per Serguei request)

sspitsyn reviewed Oct 15, 2024

View reviewed changes

src/hotspot/os/windows/attachListener_windows.cpp Outdated Show resolved Hide resolved

sspitsyn reviewed Oct 15, 2024

View reviewed changes

src/hotspot/os/windows/attachListener_windows.cpp Outdated Show resolved Hide resolved

sspitsyn reviewed Oct 15, 2024

View reviewed changes

src/hotspot/os/windows/attachListener_windows.cpp Outdated Show resolved Hide resolved

sspitsyn reviewed Oct 15, 2024

View reviewed changes

alexmenkov added 2 commits October 15, 2024 15:24

feedback

e248067

updated comment

86bd1cd

sspitsyn approved these changes Oct 16, 2024

View reviewed changes

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

kevinjwalls reviewed Oct 22, 2024

View reviewed changes

kevinjwalls reviewed Oct 24, 2024

View reviewed changes

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

kevinjwalls reviewed Oct 24, 2024

View reviewed changes

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

typos

334e735

openjdk bot removed the ready Pull request is ready to be integrated label Oct 24, 2024

renamed getVersion command

9e6a34e

alexmenkov changed the title ~~8339289: Parameter size mismatch between client and VM sides of the Attach API - Windows~~ 8339289: Enhance Attach API to support arbitrary length arguments - Windows Oct 24, 2024

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Oct 24, 2024

kevinjwalls approved these changes Oct 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 24, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 25, 2024

openjdk bot closed this Oct 25, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 25, 2024

alexmenkov deleted the attach_v2 branch October 25, 2024 18:08

alexmenkov mentioned this pull request Nov 19, 2024

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

Closed

3 tasks

varada1110 mentioned this pull request Mar 27, 2025

8352392: AIX: implement attach API v2 and streaming output #24177

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8339289: Enhance Attach API to support arbitrary length arguments - Windows #20782

8339289: Enhance Attach API to support arbitrary length arguments - Windows #20782

alexmenkov commented Aug 30, 2024 •

edited by openjdk bot

Loading

alexmenkov commented Aug 30, 2024

bridgekeeper bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024 •

edited

Loading

openjdk bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024

mlbridge bot commented Aug 30, 2024 •

edited

Loading

dcubed-ojdk commented Aug 30, 2024

dcubed-ojdk commented Aug 30, 2024

alexmenkov commented Aug 30, 2024

alexmenkov commented Sep 23, 2024

sspitsyn left a comment

sspitsyn left a comment •

edited

Loading

sspitsyn left a comment

kevinjwalls Oct 22, 2024

alexmenkov Oct 22, 2024

kevinjwalls commented Oct 24, 2024

alexmenkov commented Oct 24, 2024

alexmenkov commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

8339289: Enhance Attach API to support arbitrary length arguments - Windows #20782

8339289: Enhance Attach API to support arbitrary length arguments - Windows #20782

Conversation

alexmenkov commented Aug 30, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

alexmenkov commented Aug 30, 2024

bridgekeeper bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024 • edited Loading

openjdk bot commented Aug 30, 2024

openjdk bot commented Aug 30, 2024

mlbridge bot commented Aug 30, 2024 • edited Loading

Webrevs

dcubed-ojdk commented Aug 30, 2024

dcubed-ojdk commented Aug 30, 2024

alexmenkov commented Aug 30, 2024

alexmenkov commented Sep 23, 2024

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn left a comment • edited Loading

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

kevinjwalls Oct 22, 2024

Choose a reason for hiding this comment

alexmenkov Oct 22, 2024

Choose a reason for hiding this comment

kevinjwalls commented Oct 24, 2024

alexmenkov commented Oct 24, 2024

alexmenkov commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

openjdk bot commented Oct 25, 2024

alexmenkov commented Aug 30, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 30, 2024 •

edited

Loading

mlbridge bot commented Aug 30, 2024 •

edited

Loading

sspitsyn left a comment •

edited

Loading