RFR: 8289561: java/lang/instrument/NativeMethodPrefixAgent.java fails with "ERROR: Injection failure: java.lang.UnsupportedOperationException: Records requires ASM8"

2022-10-05 Thread Alex Menkov
Test failure is a duplicate of 
[JDK-8284777](https://bugs.openjdk.org/browse/JDK-8284777), but the test needs 
to be updated:
- the test instruments all loaded classes, so need to updated ASM version to 
support records and permits;
  Instrumentor.addNativeMethodTrackingInjection is used only by this test;
- return empty array from transforming does not cause test failure, added code 
to handle agent errors

-

Commit messages:
 - Fixed test

Changes: https://git.openjdk.org/jdk/pull/10589/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10589&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8289561
  Stats: 18 lines in 3 files changed: 12 ins; 1 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/10589.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/10589/head:pull/10589

PR: https://git.openjdk.org/jdk/pull/10589


Re: RFR: 8298380: Clean up redundant array length checks in JDK code base

2022-12-08 Thread Alex Menkov
On Thu, 8 Dec 2022 12:37:17 GMT, Sergey Tsypanov  wrote:

> Newer version of IntelliJ IDEA introduces new 
> [inspection](https://youtrack.jetbrains.com/issue/IDEA-301797/IDEA-should-report-redundant-array-length-check-in-certain-cases)
>  detecting redundant array length check in snippets like
> 
>  void iterate(T[] items) {
>   if (items.length == 0) {
> return;
>   }
>   for (T item : items) {
> //...
>   }
> }
> 
> Here
> 
> if (items.length == 0) {
>   return;
> }
> 
> is redundant and can be removed as length check is performed by for-each loop.

Marked as reviewed by amenkov (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/11589


Re: RFR: 8292350: Use static methods for hashCode/toString primitives

2022-08-19 Thread Alex Menkov
On Wed, 10 Aug 2022 08:01:41 GMT, Andrey Turbanov  wrote:

> It's a bit shorter and clearer.

Marked as reviewed by amenkov (Reviewer).

-

PR: https://git.openjdk.org/jdk/pull/9816


Re: RFR: 8309334: ProcessTools.main() does not properly set thread names when using the virtual thread wrapper

2023-06-02 Thread Alex Menkov
On Fri, 2 Jun 2023 21:30:46 GMT, Chris Plummer  wrote:

> Normally when a virtual thread wrapper is used to run a test, the main thread 
> is renamed to "old-m-a-i-n" and the new virtual thread that will act as the 
> main thread is named "main". Neither is being done by `ProcessTools.main()`. 
> This can cause problems for tests that expect the main thread that the test 
> is running in to be called "main". It is instead left unnamed. This is 
> causing the following 4 tests to fail:
> 
> com/sun/jdi/JdbMethodExitTest.java
> com/sun/jdi/JdbStepTest.java
> com/sun/jdi/JdbStopThreadTest.java
> com/sun/jdi/JdbStopThreadidTest.java
> 
> These tests also fail due to 
> [JDK-8309397](https://bugs.openjdk.org/browse/JDK-8309397), which will be 
> fixed after this CR, and also com/sun/jdi/JdbMethodExitTest.java fails due to 
> [JDK-8309396](https://bugs.openjdk.org/browse/JDK-8309396), which will also 
> subsequently be fixed.
> 
> Note this fix messed up one runtime test. It was expecting an exception 
> message to mention the "main" thread rather than "old-m-a-i-n". Loosening the 
> exception message matching pattern a bit solved the problem.
> 
> Testing was done by running all of tier1 and tier5.

Marked as reviewed by amenkov (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14292#pullrequestreview-1458569741


RFR: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library

2024-02-21 Thread Alex Menkov
VirtualMachine.loadAgentPath/loadAgentLibrary can fail with AgentLoadException 
in 2 cases:
- attach listener returns error; in the case the exception is thrown from 
HotSpotVirtualMachine.processCompletionStatus (called from 
HotSpotVirtualMachine.execute);
- attach listener returns success, but reply does not contain Agent_onAttach 
return code ("return code: %d" message).

before jdk21 if attach listener fails to load required library, it returned 
error (case 1)
from jdk21 attach listener always returns success (case 2)
Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary read 
only single line of the response message, so exception message didn't contain 
error details.

The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole 
response message.
Walking through the code, fixed some other minor things in attach listener and 
attach API implementation (commented in the code)

Testing:
  - test/jdk/com/sun/tools;
  - tier1,tier2,tier5-svc

-

Commit messages:
 - fix

Changes: https://git.openjdk.org/jdk/pull/17954/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17954&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325530
  Stats: 146 lines in 6 files changed: 106 ins; 16 del; 24 mod
  Patch: https://git.openjdk.org/jdk/pull/17954.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17954/head:pull/17954

PR: https://git.openjdk.org/jdk/pull/17954


Re: RFR: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library

2024-02-21 Thread Alex Menkov
On Wed, 21 Feb 2024 21:13:36 GMT, Alex Menkov  wrote:

> VirtualMachine.loadAgentPath/loadAgentLibrary can fail with 
> AgentLoadException in 2 cases:
> - attach listener returns error; in the case the exception is thrown from 
> HotSpotVirtualMachine.processCompletionStatus (called from 
> HotSpotVirtualMachine.execute);
> - attach listener returns success, but reply does not contain Agent_onAttach 
> return code ("return code: %d" message).
> 
> before jdk21 if attach listener fails to load required library, it returned 
> error (case 1)
> from jdk21 attach listener always returns success (case 2)
> Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary 
> read only single line of the response message, so exception message didn't 
> contain error details.
> 
> The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole 
> response message.
> Walking through the code, fixed some other minor things in attach listener 
> and attach API implementation (commented in the code)
> 
> Testing:
>   - test/jdk/com/sun/tools;
>   - tier1,tier2,tier5-svc

src/hotspot/share/prims/jvmtiAgentList.cpp line 127:

> 125: }
> 126: 
> 127: void JvmtiAgentList::add(const char* name, const char* options, bool 
> absolute_path) {

`options` parameter should be const

src/hotspot/share/prims/jvmtiAgentList.cpp line 131:

> 129: }
> 130: 
> 131: void JvmtiAgentList::add_xrun(const char* name, const char* options, 
> bool absolute_path) {

`options` parameter should be const

src/hotspot/share/prims/jvmtiAgentList.cpp line 201:

> 199: 
> 200: // Invokes Agent_OnAttach for agents loaded dynamically during runtime.
> 201: jint JvmtiAgentList::load_agent(const char* agent_name, const char* 
> absParam,

The method never returns error, dropped value return type

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line 
402:

> 400: }
> 401: 
> 402: // Special-case the "load" command so that the right 
> exception is

We had special logic for "load" command in 2 places (here and in the 
loadAgentLibrary method)
For simplification moved all logic to loadAgentLibrary()

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1498320621
PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1498320775
PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1498321476
PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1498333650


Re: RFR: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library

2024-02-21 Thread Alex Menkov
On Wed, 21 Feb 2024 21:16:46 GMT, Alex Menkov  wrote:

>> VirtualMachine.loadAgentPath/loadAgentLibrary can fail with 
>> AgentLoadException in 2 cases:
>> - attach listener returns error; in the case the exception is thrown from 
>> HotSpotVirtualMachine.processCompletionStatus (called from 
>> HotSpotVirtualMachine.execute);
>> - attach listener returns success, but reply does not contain Agent_onAttach 
>> return code ("return code: %d" message).
>> 
>> before jdk21 if attach listener fails to load required library, it returned 
>> error (case 1)
>> from jdk21 attach listener always returns success (case 2)
>> Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary 
>> read only single line of the response message, so exception message didn't 
>> contain error details.
>> 
>> The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole 
>> response message.
>> Walking through the code, fixed some other minor things in attach listener 
>> and attach API implementation (commented in the code)
>> 
>> Testing:
>>   - test/jdk/com/sun/tools;
>>   - tier1,tier2,tier5-svc
>
> src/hotspot/share/prims/jvmtiAgentList.cpp line 201:
> 
>> 199: 
>> 200: // Invokes Agent_OnAttach for agents loaded dynamically during runtime.
>> 201: jint JvmtiAgentList::load_agent(const char* agent_name, const char* 
>> absParam,
> 
> The method never returns error, dropped value return type

only 1 caller (load_agent() from attachListener) extract `absParam` from attach 
command, other pass constants ("true" or "false")
Moved conversion from string to bool there.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17954#discussion_r1498327164


Re: RFR: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library [v2]

2024-02-21 Thread Alex Menkov
> VirtualMachine.loadAgentPath/loadAgentLibrary can fail with 
> AgentLoadException in 2 cases:
> - attach listener returns error; in the case the exception is thrown from 
> HotSpotVirtualMachine.processCompletionStatus (called from 
> HotSpotVirtualMachine.execute);
> - attach listener returns success, but reply does not contain Agent_onAttach 
> return code ("return code: %d" message).
> 
> before jdk21 if attach listener fails to load required library, it returned 
> error (case 1)
> from jdk21 attach listener always returns success (case 2)
> Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary 
> read only single line of the response message, so exception message didn't 
> contain error details.
> 
> The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole 
> response message.
> Walking through the code, fixed some other minor things in attach listener 
> and attach API implementation (commented in the code)
> 
> Testing:
>   - test/jdk/com/sun/tools;
>   - tier1,tier2,tier5-svc

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  uncaught error

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17954/files
  - new: https://git.openjdk.org/jdk/pull/17954/files/19f0822e..0304bae2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=17954&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17954&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17954.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17954/head:pull/17954

PR: https://git.openjdk.org/jdk/pull/17954


RFR: JDK-8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case

2024-02-22 Thread Alex Menkov
The change updates the test to throw an exception if expected 
AgentLoadException is not thrown

-

Commit messages:
 - fix

Changes: https://git.openjdk.org/jdk/pull/17971/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17971&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8326525
  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17971.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17971/head:pull/17971

PR: https://git.openjdk.org/jdk/pull/17971


RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE

2024-03-07 Thread Alex Menkov
RecordComponent class has _attributes_count field.
The only user of the field is JvmtiClassFileReconstituter. Incorrect value of 
the field causes producing incorrect data for Record attribute.
Parsing Record attribute ClassFileParser skips unknown attributes and may skip 
RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
The fix updates ClassFileParser to calculate correct attributes_count.

Testing: 
- tier1,tier2,hs-tier5-svc;
 - redefineClasses/retransformClasses tests:
   - test/jdk/java/lang/instrument
   - test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
   - test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
   - test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

-

Commit messages:
 - fix

Changes: https://git.openjdk.org/jdk/pull/18161/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18161&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8315575
  Stats: 133 lines in 2 files changed: 131 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18161.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161

PR: https://git.openjdk.org/jdk/pull/18161


Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v2]

2024-03-11 Thread Alex Menkov
> RecordComponent class has _attributes_count field.
> The only user of the field is JvmtiClassFileReconstituter. Incorrect value of 
> the field causes producing incorrect data for Record attribute.
> Parsing Record attribute ClassFileParser skips unknown attributes and may 
> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
> The fix updates ClassFileParser to calculate correct attributes_count.
> 
> Testing: 
> - tier1,tier2,hs-tier5-svc;
>  - redefineClasses/retransformClasses tests:
>- test/jdk/java/lang/instrument
>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  typo

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18161/files
  - new: https://git.openjdk.org/jdk/pull/18161/files/5f1d0116..1ad6997d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18161&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18161&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18161.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161

PR: https://git.openjdk.org/jdk/pull/18161


Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v2]

2024-03-11 Thread Alex Menkov
On Sat, 9 Mar 2024 03:29:26 GMT, Guoxiong Li  wrote:

>> Alex Menkov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   typo
>
> test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 27:
> 
>> 25:  * @test
>> 26:  * @bug 8315575
>> 27:  * @summary test that records with invisible annotation can be 
>> retfansformed
> 
> Typo `retfansformed`.

Fixed

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1520552930


Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v3]

2024-03-12 Thread Alex Menkov
> RecordComponent class has _attributes_count field.
> The only user of the field is JvmtiClassFileReconstituter. Incorrect value of 
> the field causes producing incorrect data for Record attribute.
> Parsing Record attribute ClassFileParser skips unknown attributes and may 
> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations.
> Also annotations can be changed (added/removed) by class redefinition.
> The fix removes attributes_count from RecordComponent; 
> JvmtiClassFileReconstituter calculates correct attributes_count generating 
> class bytes.
> 
> Testing: 
> - tier1,tier2,hs-tier5-svc;
>  - redefineClasses/retransformClasses tests:
>- test/jdk/java/lang/instrument
>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  removed attributes_count from RecordComponent

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18161/files
  - new: https://git.openjdk.org/jdk/pull/18161/files/1ad6997d..f82e432a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18161&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18161&range=01-02

  Stats: 24 lines in 4 files changed: 3 ins; 14 del; 7 mod
  Patch: https://git.openjdk.org/jdk/pull/18161.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18161/head:pull/18161

PR: https://git.openjdk.org/jdk/pull/18161


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v14]

2024-10-28 Thread Alex Menkov
On Sun, 27 Oct 2024 21:45:08 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   remove newly added trailing space

Marked as reviewed by amenkov (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21397#pullrequestreview-2400431422


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v11]

2024-10-25 Thread Alex Menkov
On Thu, 24 Oct 2024 14:52:27 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: removed asserts from continuationFreezeThaw.cpp again

src/hotspot/share/prims/jvmtiEnvBase.cpp line 667:

> 665: 
> 666: javaVFrame*
> 667: JvmtiEnvBase::check_and_skip_hidden_frames(bool is_in_VTMS_transition, 
> javaVFrame* jvf) {

reworked function looks much better! Now it's clear what the function does and 
I have a question what it should do.
The function checks `@JvmtiMountTransition` annotation first even if the thread 
is in transition.
If `@JvmtiMountTransition` is present, the code doesn't care about 
`@ChangesCurrentThread` (and doesn't case about `@JvmtiMountTransition` if 
in_transition + `@ChangesCurrentThread`).
But we have 2 methods in VirtualThread class (`switchToCarrierThread()` and 
`switchToVirtualThread()`) with both annotations.
So if the function is called when `switchToCarrierThread` is on top, the 
function skips this frame, but if the thread calls some other function without 
`@JvmtiMountTransition` annotation from `switchToCarrierThread`, the function 
returns `switchToCarrierThread`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1815790409


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v16]

2024-10-29 Thread Alex Menkov
On Tue, 29 Oct 2024 08:35:27 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: one more correction in the annotated method description

Marked as reviewed by amenkov (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21397#pullrequestreview-2402752520


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v30]

2024-11-05 Thread Alex Menkov
On Wed, 6 Nov 2024 00:58:10 GMT, David Holmes  wrote:

> I think you may be throwing the baby out with the bath water when it comes to 
> `__stdcall`. It may be that 32-bit requires `__stdcall` but I don't see 
> anything that states `__stdcall` is ONLY for 32-bit!

https://learn.microsoft.com/en-us/cpp/cpp/stdcall?view=msvc-170
`On ARM and x64 processors, __stdcall is accepted and ignored by the compiler; 
on ARM and x64 architectures, by convention, arguments are passed in registers 
when possible, and subsequent arguments are passed on the stack.`

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2458534929


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v12]

2024-10-25 Thread Alex Menkov
On Fri, 25 Oct 2024 20:52:29 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: introduce new annotation @JvmtiHideEvents and use it in 
> VirtualThread/Continuation classes to disallow FramePop requests

src/java.base/share/classes/jdk/internal/vm/annotation/JvmtiHideEvents.java 
line 2:

> 1: /*
> 2:  * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights 
> reserved.

(c) 2024

src/java.base/share/classes/jdk/internal/vm/annotation/JvmtiMountTransition.java
 line 38:

> 36:  *
> 37:  * @implNote
> 38:  * This annotation is only used for the VirtualThread notifyJvmti*  
> methods.

What about VirtualThread.switchToCarrierThread and 
VirtualThread.switchToVirtualThread ? They also have the annotation.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1817470128
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1817469168


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v3]

2024-10-15 Thread Alex Menkov
On Wed, 9 Oct 2024 22:58:33 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Disallow NotifyFramePop for 
> enter/enter0/VirtualThread.run/VThreadContinuation.run

src/hotspot/share/prims/jvmtiEnvBase.cpp line 692:

> 690:   if (jt->is_in_VTMS_transition()) {
> 691: jvf = check_and_skip_hidden_frames(jt->is_in_VTMS_transition(), jvf);
> 692:   } else if (is_virtual) { // filter out pure continuations

Not sure I follow the logic here.
As far as I understand yield/yield need to be filtered out only for unmounted 
virtual threads. But `is_virtual` here means we have mounted VT?
Looks like almost all callers call this method only if 
`jt->is_in_JTMS_transilition()` is true
Exception is a call from `get_vthread_jvf()` when vthread is mounted.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 703:

> 701:   if (java_lang_VirtualThread::is_instance(vthread)) { // paranoid check 
> for safety
> 702: if (java_lang_Thread::is_in_VTMS_transition(vthread)) {
> 703:   jvf = 
> check_and_skip_hidden_frames(java_lang_Thread::is_in_VTMS_transition(vthread),
>  jvf);

it's just checked that `java_lang_Thread::is_in_VTMS_transition(vthread)` is 
true
Suggestion:

  jvf = check_and_skip_hidden_frames(true, jvf);

src/hotspot/share/prims/jvmtiEnvBase.cpp line 2025:

> 2023:   // - it is a good invariant when a thread's handshake can't be 
> impacted by a VTMS transition
> 2024:   // - there is no mechanism to disable transitions of a specific 
> carrier thread yet
> 2025:   JvmtiVTMSTransitionDisabler disabler(is_virtual ? target : nullptr); 
> // nullptr is to disable all

We have a number of places with the same issue - `JvmtiVTMSTransitionDisabler 
disabler(target)` when target thread can be virtual or platform.
I think they need to be fixed all together (and most likely as a separate 
issue).
Maybe it would be better to fix disabler itself (check if the thread provided 
is platform and disable transitions for all threads in the case). Then there is 
no need to update all this places when (if) disabling for single platform 
thread is implemented

src/hotspot/share/prims/jvmtiExport.cpp line 1814:

> 1812:   }
> 1813:   // pure continuations have no VTMS transitions but they use methods 
> annotated with JvmtiMountTransition
> 1814:   if ((mh->jvmti_mount_transition() && state->is_virtual()) || 
> thread->is_in_any_VTMS_transition()) {

Could you please explain the change (and other similar changes in 
jvmtiExport.cpp)
Before events were not posted for any `@JvmtiMountTransition` methods, and now 
only for virtual threads? I.e. events for `@JvmtiMountTransition` methods of 
carrier threads are posted?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801933054
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801953309
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801992736
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802034030


Re: RFR: 8341273: JVMTI is not properly hiding some continuation related methods [v3]

2024-10-15 Thread Alex Menkov
On Wed, 9 Oct 2024 22:58:33 GMT, Serguei Spitsyn  wrote:

>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition 
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>> 
>> Testing:
>>  - Verified with new test `vthread/CheckHiddenFrames`
>>  - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Disallow NotifyFramePop for 
> enter/enter0/VirtualThread.run/VThreadContinuation.run

src/hotspot/share/prims/jvmtiEnvBase.cpp line 588:

> 586:   // There should not be any VTMS transition here. This is for safety.
> 587:   if (java_thread->is_in_VTMS_transition()) {
> 588: jvf = JvmtiEnvBase::check_and_skip_hidden_frames(java_thread, jvf);

The code now checks `java_thread->is_in_VTMS_transition()`, so it may be 
simplified to
Suggestion:

jvf = JvmtiEnvBase::check_and_skip_hidden_frames(true, jvf);

src/hotspot/share/prims/jvmtiEnvBase.cpp line 691:

> 689: 
> 690:   if (jt->is_in_VTMS_transition()) {
> 691: jvf = check_and_skip_hidden_frames(jt->is_in_VTMS_transition(), jvf);

Suggestion:

jvf = check_and_skip_hidden_frames(true, jvf);

src/hotspot/share/prims/jvmtiEnvBase.cpp line 753:

> 751: // Skip hidden frames only for carrier threads
> 752: // which are in non-temporary VTMS transition.
> 753: jvf = check_and_skip_hidden_frames(jt, jvf);

Can be
Suggestion:

jvf = check_and_skip_hidden_frames(true, jvf);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802038615
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802039013
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802039786


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v31]

2024-11-07 Thread Alex Menkov
On Fri, 8 Nov 2024 02:13:09 GMT, David Holmes  wrote:

> Can someone confirm that use of `__stdcall` has no affect on name 
> decorations, as there is no mention here about anything being ignored:
> 
> https://learn.microsoft.com/en-us/cpp/build/reference/decorated-names?view=msvc-170
> 
> I would have expected that if argument passing needs to use the stack then 
> the decorated name would still need to encode that somehow.

In the page you mentioned:

 Format of a C decorated name
The form of decoration for a C function depends on the calling convention used 
in its declaration, as shown in the following table. It's also the decoration 
format that's used when C++ code is declared to have extern "C" linkage. The 
default calling convention is __cdecl. **In a 64-bit environment, C or extern 
"C" functions are only decorated when using the __vectorcall calling 
convention**.

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2463636430


Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v31]

2024-11-08 Thread Alex Menkov
On Fri, 8 Nov 2024 05:29:05 GMT, David Holmes  wrote:

> > In the page you mentioned:
> 
> @alexmenkov that is for C functions, not C++.

And also for `extern "C"` (AFAIU we export all C++ functions as extern "C")

-

PR Comment: https://git.openjdk.org/jdk/pull/21744#issuecomment-2464243142


Re: RFR: 8343703: Symbol name cleanups after JEP 479 [v2]

2024-11-27 Thread Alex Menkov
On Wed, 27 Nov 2024 05:20:16 GMT, David Holmes  wrote:

>> After JEP 479 ([JDK-8339783](https://bugs.openjdk.org/browse/JDK-8339783) 
>> was integrated, the handling of certain symbol lookup code can be 
>> simplified. The old code needed to support 32-bit Windows, where names had a 
>> trailing `@`. When this special case now is removed, some 
>> streamlining is possible. Specifically these arrays:
>> 
>> #define JNI_ONLOAD_SYMBOLS {"JNI_OnLoad"}
>> #define JNI_ONUNLOAD_SYMBOLS {"JNI_OnUnload"}
>> #define JVM_ONLOAD_SYMBOLS {"JVM_OnLoad"}
>> #define AGENT_ONLOAD_SYMBOLS {"Agent_OnLoad"}
>> #define AGENT_ONUNLOAD_SYMBOLS {"Agent_OnUnload"}
>> #define AGENT_ONATTACH_SYMBOLS {"Agent_OnAttach"}
>> 
>> are all singletons and so the actual strings can just be inlined directly 
>> into the code that uses them.
>> 
>> Testing:
>> - GHA
>> - Tiers 1-4 sanity
>> - 
>> Thanks
>
> David Holmes has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update src/java.base/share/native/libjava/NativeLibraries.c
>   
>   Co-authored-by: Alex Menkov <69548902+alexmen...@users.noreply.github.com>

Marked as reviewed by amenkov (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/22380#pullrequestreview-2465773079


Re: RFR: 8343703: Symbol name cleanups after JEP 479

2024-11-26 Thread Alex Menkov
On Tue, 26 Nov 2024 06:36:44 GMT, David Holmes  wrote:

> After JEP 479 ([JDK-8339783](https://bugs.openjdk.org/browse/JDK-8339783) was 
> integrated, the handling of certain symbol lookup code can be simplified. The 
> old code needed to support 32-bit Windows, where names had a trailing 
> `@`. When this special case now is removed, some streamlining is 
> possible. Specifically these arrays:
> 
> #define JNI_ONLOAD_SYMBOLS {"JNI_OnLoad"}
> #define JNI_ONUNLOAD_SYMBOLS {"JNI_OnUnload"}
> #define JVM_ONLOAD_SYMBOLS {"JVM_OnLoad"}
> #define AGENT_ONLOAD_SYMBOLS {"Agent_OnLoad"}
> #define AGENT_ONUNLOAD_SYMBOLS {"Agent_OnUnload"}
> #define AGENT_ONATTACH_SYMBOLS {"Agent_OnAttach"}
> 
> are all singletons and so the actual strings can just be inlined directly 
> into the code that uses them.
> 
> Testing:
> - GHA
> - Tiers 1-4 sanity
> - 
> Thanks

Marked as reviewed by amenkov (Reviewer).

src/java.base/share/native/libjava/NativeLibraries.c line 76:

> 74: 
> 75: // sym + '_' + cname + '\0'
> 76: if ((len = (cname != NULL ? (strlen(cname) + 1) : 0) + strlen(sym) + 
> 1) >

To match the updated comment:
Suggestion:

if ((len = strlen(sym) + (cname != NULL ? (strlen(cname) + 1) : 0) + 1) >

-

PR Review: https://git.openjdk.org/jdk/pull/22380#pullrequestreview-2463375580
PR Review Comment: https://git.openjdk.org/jdk/pull/22380#discussion_r1859629230


Integrated: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest

2024-12-17 Thread Alex Menkov
On Fri, 13 Dec 2024 02:21:42 GMT, Alex Menkov  wrote:

> In some circumstances ClassFileTransformer.transform can get 
> ClassCircularityError or LinkageError concatenating strings (see JDK-8264667, 
> JDK-8262002).
> VerifyLocalVariableTableOnRetransformTest fails sometimes on Oracle CI.
> The fix adds handling of the errors to get information for analysis.

This pull request has now been integrated.

Changeset: c0f0b8e5
Author:Alex Menkov 
URL:   
https://git.openjdk.org/jdk/commit/c0f0b8e5f4d83ae7dd7e67930c19134855e5e97b
Stats: 21 lines in 1 file changed: 15 ins; 0 del; 6 mod

8346151: Add transformer error logging to 
VerifyLocalVariableTableOnRetransformTest

Reviewed-by: cjplummer, sspitsyn

-

PR: https://git.openjdk.org/jdk/pull/22727


RFR: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest

2024-12-12 Thread Alex Menkov
In some circumstances ClassFileTransformer.transform can get 
ClassCircularityError or LinkageError concatenating strings (see JDK-8264667, 
JDK-8262002).
VerifyLocalVariableTableOnRetransformTest fails sometimes on Oracle CI.
The fix adds handling of the errors to get information for analysis.

-

Commit messages:
 - test update

Changes: https://git.openjdk.org/jdk/pull/22727/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22727&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8346151
  Stats: 20 lines in 1 file changed: 15 ins; 0 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/22727.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/22727/head:pull/22727

PR: https://git.openjdk.org/jdk/pull/22727


Re: RFR: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest [v2]

2024-12-13 Thread Alex Menkov
> In some circumstances ClassFileTransformer.transform can get 
> ClassCircularityError or LinkageError concatenating strings (see JDK-8264667, 
> JDK-8262002).
> VerifyLocalVariableTableOnRetransformTest fails sometimes on Oracle CI.
> The fix adds handling of the errors to get information for analysis.

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  feedback

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/22727/files
  - new: https://git.openjdk.org/jdk/pull/22727/files/7665ba02..2cbdc9b9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=22727&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22727&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/22727.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/22727/head:pull/22727

PR: https://git.openjdk.org/jdk/pull/22727


Re: RFR: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest [v2]

2024-12-13 Thread Alex Menkov
On Fri, 13 Dec 2024 20:49:22 GMT, Chris Plummer  wrote:

>> Alex Menkov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   feedback
>
> test/jdk/java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.java 
> line 259:
> 
>> 257: + "' of " + classfileBuffer.length + " bytes.");
>> 258: } catch (Throwable t) {
>> 259: // try to log, save the error for handling by main 
>> thread if the logging fails
> 
> Suggestion:
> 
> // Try to log. Save the error for handling by main thread if 
> the logging fails.

Done

> test/jdk/java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.java 
> line 261:
> 
>> 259: // try to log, save the error for handling by main 
>> thread if the logging fails
>> 260: try {
>> 261: transformerException.printStackTrace();
> 
> Isn't this suppose to be `t.printStackTrace()`?

Good catch!
Fixed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/22727#discussion_r1884580477
PR Review Comment: https://git.openjdk.org/jdk/pull/22727#discussion_r1884580327


Re: RFR: 8346151: Add transformer error logging to VerifyLocalVariableTableOnRetransformTest [v3]

2024-12-13 Thread Alex Menkov
> In some circumstances ClassFileTransformer.transform can get 
> ClassCircularityError or LinkageError concatenating strings (see JDK-8264667, 
> JDK-8262002).
> VerifyLocalVariableTableOnRetransformTest fails sometimes on Oracle CI.
> The fix adds handling of the errors to get information for analysis.

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  copyright update

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/22727/files
  - new: https://git.openjdk.org/jdk/pull/22727/files/2cbdc9b9..012579f1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=22727&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=22727&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/22727.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/22727/head:pull/22727

PR: https://git.openjdk.org/jdk/pull/22727


RFR: 8354461: Update tests to disable streaming output for attach tools

2025-04-15 Thread Alex Menkov
The change is a preparation step to enable attach streaming output by default.
The fix updates a number of tests which fail with timeout in tier1..tier7 when 
attach streaming output is enabled.
Details in the first comment.
Testing: tier1..7 with enabled attach streaming output

-

Commit messages:
 - tests

Changes: https://git.openjdk.org/jdk/pull/24672/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24672&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8354461
  Stats: 159 lines in 19 files changed: 44 ins; 56 del; 59 mod
  Patch: https://git.openjdk.org/jdk/pull/24672.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/24672/head:pull/24672

PR: https://git.openjdk.org/jdk/pull/24672


Re: RFR: 8354461: Update tests to disable streaming output for attach tools

2025-04-15 Thread Alex Menkov
On Tue, 15 Apr 2025 23:30:35 GMT, Alex Menkov  wrote:

> The change is a preparation step to enable attach streaming output by default.
> The fix updates a number of tests which fail with timeout in tier1..tier7 
> when attach streaming output is enabled.
> Details in the first comment.
> Testing: tier1..7 with enabled attach streaming output

The tests use the same scenario: test runs a tool (jcmd/jstack/jmap) to attach 
to the main test process and redirects the tool output for analysis.
We have 2 buffers here:
1. attach channel (socket or pipe depending on platform) - attach operation 
handler writes, the tool reads.
2. tool redirection buffer - the tool writes, test reads.
When attach operation is executes at safepoint, attach streaming output is 
enabled and the tool output is lengthy we can get both buffers full and the 
test hung:
- attach handler blocks on write operation (tool need to read from the 
socket/pipe);
- the tool reads data from attach channel and blocks writing the data to its 
stdout (need to read from redirected stream);
- test redirector reader cannot read because the VM is at safepoint executing 
attach operation.
To avoid the deadlocks the test are updated to disable attach streaming output 
(attach operation output is buffered and is sent after the operation is 
completed).

-

PR Comment: https://git.openjdk.org/jdk/pull/24672#issuecomment-2807765324


RFR: 8357650: ThreadSnapshot to take snapshot of thread for thread dumps

2025-05-23 Thread Alex Menkov
This is first (hotspot) part of the update for 
`HotSpotDiagnosticMXBean.dumpThreads` and `jcmd Thread.dump_to_file` to include 
lock information in thread dumps (JDK-8356870).
The update has been split into parts to simplify reviewing.
The fix contains an implementation of `jdk.internal.vm.ThreadSnapshot` class to 
gather required information about a thread.
Second (dependent) part includes changes in 
`HotSpotDiagnosticMXBean.dumpThreads`/`jcmd Thread.dump_to_file`, spec updates 
and tests for the functionality.

Testing: new `HotSpotDiagnosticMXBean.dumpThreads`/`jcmd Thread.dump_to_file` 
functionality was tested in loom repo;
  sanity tier1 (this fix only)

-

Commit messages:
 - initial fix

Changes: https://git.openjdk.org/jdk/pull/25425/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25425&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8357650
  Stats: 736 lines in 8 files changed: 733 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/25425.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/25425/head:pull/25425

PR: https://git.openjdk.org/jdk/pull/25425