Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v6]

[Alan Bateman]

On Wed, 31 May 2023 20:37:23 GMT, Chris Plummer  wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 14 additional 
>> commits since the last revision:
>> 
>>  - Allow for warning to be skipped when same agent loaded a 
>> second/subsequent time
>>  - Merge
>>  - Tweak javadoc, update test to use more test infra
>>  - Merge
>>  - Merge
>>  - Refresh package description
>>  - Merge
>>  - Tweak docs
>>  - Merge
>>  - Draft docs changes
>>  - ... and 4 more: https://git.openjdk.org/jdk/compare/5a6c8363...a6d3c23c
>
> src/hotspot/share/prims/jvmtiAgentList.cpp line 231:
> 
>> 229:if (agent->is_static_lib() && agent->is_loaded()) {
>> 230:  return true;
>> 231:}
> 
> This doesn't make sense to me. If  you pass in `null` for `os_lib`, then we 
> return true if any loaded static lib is found. Is this an attempt to limit 
> the warning to just the first static lib that is loaded? Also, why would 
> `null` ever be passed in if there wasn't at least one static lib. Some 
> clarify comments would be useful.

load_agent_from_executable has a comment to explain how statically linked 
agents are started, that's why it needs to use agent->is_static_lib().&& 
agent->is_loaded() here. There isn't currently a way to test this but there is 
other work going to support static builds so it might be possible to write some 
automated tests at that point.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212613094

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Alan Bateman]

On Thu, 1 Jun 2023 05:20:31 GMT, Serguei Spitsyn  wrote:

>> src/hotspot/share/prims/jvmtiAgent.cpp line 512:
>> 
>>> 510: 
>>> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the 
>>> command line
>>> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && 
>>> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {
>> 
>> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. 
>> Isn't the library always already loaded by the time we get here (the assert 
>> below seems to imply that)? If so, wouldn't it already be in the list? If 
>> it's not in the list yet, perhaps a comment explaining why would be helpful 
>> here.
>
> It looks like you are right.
> There is also and assert at line 519:
> `519   assert(agent->is_loaded(), "invariant");`
> 
> So, the agent has to be loaded if we got to the line 512.
> Also, there is a statement at line 507 (before line 512):
> `507agent->set_os_lib(library);`

> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. 
> Isn't the library always already loaded by the time we get here (the assert 
> below seems to imply that)? 

JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and 
adds it to the agent list if is succeeds. The test that you are looking is 
checking the list of already loaded agents. Maybe the function name "is_loaded" 
is the confusion here, maybe a better name is needed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212610098

Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Tobias Hartmann]

On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen  wrote:

>> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes 
>> I'd appreciate if this was considered trivial.
>
> Johan Sjölen has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Align
>  - Suggestions

What's the plan now to prevent re-introducing `NULL`?

-

PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571360076

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Serguei Spitsyn]

On Wed, 31 May 2023 20:21:15 GMT, Chris Plummer  wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 17 additional 
>> commits since the last revision:
>> 
>>  - Add impl note to document the XX option
>>  - Cleanup
>>  - Merge
>>  - Allow for warning to be skipped when same agent loaded a 
>> second/subsequent time
>>  - Merge
>>  - Tweak javadoc, update test to use more test infra
>>  - Merge
>>  - Merge
>>  - Refresh package description
>>  - Merge
>>  - ... and 7 more: https://git.openjdk.org/jdk/compare/a3c18c96...2d9d5922
>
> src/hotspot/share/prims/jvmtiAgent.cpp line 512:
> 
>> 510: 
>> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the 
>> command line
>> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && 
>> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {
> 
> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. 
> Isn't the library always already loaded by the time we get here (the assert 
> below seems to imply that)? If so, wouldn't it already be in the list? If 
> it's not in the list yet, perhaps a comment explaining why would be helpful 
> here.

It looks like you are right.
There is also and assert at line 519:
`519   assert(agent->is_loaded(), "invariant");`

So, the agent has to be loaded if we got to the line 512.
Also, there is a statement at line 507 (before line 512):
`507agent->set_os_lib(library);`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212584288

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v3]

[Serguei Spitsyn]

On Thu, 1 Jun 2023 02:09:42 GMT, David Holmes  wrote:

> A change in behaviour like this requires a CSR request - the code has been 
> doing the wrong thing for a long time now!

Thank you for the comment. It was not fully clear if a CSR is needed in this 
case.
Created CSR (it is listed in the PR description).

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1571263870

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Serguei Spitsyn]

On Wed, 31 May 2023 20:04:04 GMT, Chris Plummer  wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 17 additional 
>> commits since the last revision:
>> 
>>  - Add impl note to document the XX option
>>  - Cleanup
>>  - Merge
>>  - Allow for warning to be skipped when same agent loaded a 
>> second/subsequent time
>>  - Merge
>>  - Tweak javadoc, update test to use more test infra
>>  - Merge
>>  - Merge
>>  - Refresh package description
>>  - Merge
>>  - ... and 7 more: https://git.openjdk.org/jdk/compare/09807c2a...2d9d5922
>
> src/hotspot/share/prims/jvmtiAgent.cpp line 512:
> 
>> 510: 
>> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the 
>> command line
>> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && 
>> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {
> 
> While looking at some code related to this, I noticed a couple of typos in 
> the pre-existing load_agent_from_executable() comment. See lines 265 
> ("cant't") and 268 (".&&"). Maybe you could clean them up.

Nit: I'd suggest to add dot at the end of comment at 511.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212499408

Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]

[David Holmes]

On Wed, 31 May 2023 12:07:49 GMT, Severin Gehwolf  wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains three additional 
>> commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8308090-tests-container-on-fly-updates
>>  - Fix whitespace
>>  - 8308090: Add container tests for on-the-fly resource quota updates
>
> Anyone willing to review this?

@jerboaa I can't really review the tests themselves but will run through our CI 
to see if they cause any problems. If not then they should be okay to add.

-

PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1571223360

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v3]

[David Holmes]

On Wed, 31 May 2023 23:39:19 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach

A change in behaviour like this requires a CSR request - the code has been 
doing the wrong thing for a long time now!

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1571218267

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[David Holmes]

Hi Jaroslav,

On 31/05/2023 9:12 pm, Jaroslav Bachorík wrote:

Dear Team,

I've been investigating the unusual JVM crashes occurring in JVMTI calls 
on a J9 JVM. During my investigation, I scrutinized the `jmethodID` 
definition closely, available here: [jmethodID 
definition](https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID ).


To paraphrase, the definition suggests that `jmethodID` identifies a 
Java method, initializer, or constructor. These identifiers, once 
returned by JVM TI functions and events, can be safely stored. However, 
when the class is unloaded, they become invalid, rendering them 
unsuitable for use.


My interpretation is that the JVMTI user should verify the validity of a 
`jmethodID` value before using it to prevent potential crashes. Would 
you agree with this interpretation?


Not quite - as you note you can't verify the jmethodID validity. What 
the user needs to do, in line with what Dan was saying, is ensure that 
they keep track of the classes to which the methods belong and keep them 
alive if necessary. Now that may be easier said than done, but that is 
the gist of it. This comes from the JNI spec:


"A field or method ID does not prevent the VM from unloading the class 
from which the ID has been derived. After the class is unloaded, the 
method or field ID becomes invalid and may not be passed to any function 
taking such an ID. The native code, therefore, must make sure to:


keep a live reference to the underlying class, or
recompute the method or field ID

if it intends to use a method or field ID for an extended period of time."

This sounds like a sensible requirement, but its practical application 
remains unclear. As far as I know, methods can be unloaded concurrently 
to the native code executing JVMTI functions. This introduces a 
potential race condition where the JVM unloads the methods during the 
check->use flow, making it only a partial solution. To complicate 
matters further, no method exists to confirm whether a `jmethodID` is valid.


Theoretically, we could monitor the `CompiledMethodUnload` event to 
track the validity state, creating a constantly expanding set of 
unloaded `jmethodID` values or a bloom filter, if one does not care 
about few potential false positives. This strategy, however, doesn't 
address the potential race condition, and it could even exacerbate it 
due to possible event delays. This delay might mistakenly validate a 
`jmethodID` value that has already been unloaded, but for which the 
event hasn't been delivered yet.


Honestly, I don't see a way to use `jmethodID` safely unless the code 
using it suspends the entire JVM and doesn't resume until it's finished 
with that `jmethodID`. Any other approach might lead to JVM crashes, as 
we've observed with J9.


Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure 
that using jmethodIDs for unloaded methods doesn't crash the JVM and 
even provides useful information. This observation has led me to 
question whether the documentation aligns with the Hotspot 
implementation, especially given that following closely the 
documentation appears to increase the risk associated with the use of 
`jmethodID` values.


There have been folk who wanted to make this area more user-friendly but 
that shouldn't be mistaken for moving towards a world where jmethodIDs 
are always safe to use.


Cheers,
David


I welcome your thoughts and perspectives on this matter.

Best regards,

Jaroslav

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Alex Menkov]

On Wed, 31 May 2023 22:57:08 GMT, Serguei Spitsyn  wrote:

> > Something went wrong after 1st merge, testing failed with OOMEInAQS (which 
> > is problem-listed)
> 
> How do you run tests? Do you run tiers 1-5 or something else as well? Please, 
> remember that the tier-5 runs the needed SVC tests with 
> `main.wrapper=virtual`.

tier1-tier5 as per David's request
initial fix (before the test were problem-listed) was tested with 
JTREG_TEST_THREAD_FACTORY=Virtual

-

PR Comment: https://git.openjdk.org/jdk/pull/14233#issuecomment-1571154252

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Serguei Spitsyn]

On Wed, 31 May 2023 11:07:02 GMT, Alan Bateman  wrote:

> Yes, maybe JvmtiAgentList::load_agent, JvmtiAgent::load, or 
> invoke_Agent_OnAttach. As part of the change to emit a warning when agent 
> code is loaded into a running VM, invoke_Agent_OnAttach needs to check if 
> EnableDynamicAgentLoading has been set on the command line, so looking at 
> this VM option in one place would be nice.

I've moved the EnableDynamicAgentLoading check to the invoke_Agent_OnAttach.
It is the same place where a warning is going to be generated as part of the 
8307478 (JEP enhancement).

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1571100330

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v3]

[Serguei Spitsyn]

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14244/files
  - new: https://git.openjdk.org/jdk/pull/14244/files/829a0e9a..be0ec0c9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14244=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=14244=01-02

  Stats: 17 lines in 4 files changed: 6 ins; 9 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/14244.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244

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

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Serguei Spitsyn]

On Wed, 31 May 2023 22:10:11 GMT, Alex Menkov  wrote:

> Something went wrong after 1st merge, testing failed with OOMEInAQS (which is 
> problem-listed)

How do you run tests? Do you run tiers 1-5 or something else as well?
Please, remember that the tier-5 runs the needed SVC tests with 
`main.wrapper=virtual`.

-

PR Comment: https://git.openjdk.org/jdk/pull/14233#issuecomment-1571070200

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v4]

[Chris Plummer]

On Tue, 23 May 2023 14:08:11 GMT, Alan Bateman  wrote:

>> test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java line 108:
>> 
>>> 106: // no warning should be printed
>>> 107: test((pid, vm) -> vm.loadAgentLibrary(JVMTI_AGENT_LIB), 
>>> "-XX:+EnableDynamicAgentLoading")
>>> 108: .shouldNotContain(message);
>> 
>> How about adding a test case for `-XX:-EnableDynamicAgentLoading`. Same 
>> thing in `testJCmdJvmtiAgentLoad()` and `testLoadJavaAgent()` below.
>
> We can't currently test `jcmd JVMTI.agent_load` with 
> `-XX:-EnableDynamicAgentLoading` due to JDK-8304438.

@AlanBateman  @sspitsyn Can this be revisited now that JDK-8304438 is being 
worked on? Perhaps the changes to JDK-8304438 can include an update to this 
test, although it all depends on which is being pushed first.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212369828

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Alex Menkov]

On Wed, 31 May 2023 21:33:13 GMT, Alex Menkov  wrote:

>> The change fixes regression from JDK-8299414.
>> There is a deadlock between JvmtiVTMSTransitionDisabler and EscapeBarrier 
>> when virtual threads are in mount/unmount transition:
>> EscapeBarrier requests deoptimization which requires thread suspension.
>> JvmtiVTMSTransitionDisabler ctor waits until all in progress VTMS 
>> transitions complete, but they cannot be completed as thread is suspended.
>> To avoid the deadlock mount/unmount transitions should be completed before 
>> EscapeBarrier stuff.
>
> Alex Menkov has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - Merge branch 'openjdk:master' into follow_ref_deadlock
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - fix
>  - unproblem-list tests
>  - fix
>  - unproblem-list tests
>  - fix

Something went wrong after 1st merge, testing failed with OOMEInAQS (which is 
problem-listed)
Remerged and rerun testing

-

PR Comment: https://git.openjdk.org/jdk/pull/14233#issuecomment-1571030428

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Leonid Mesnik]

On Wed, 31 May 2023 21:33:13 GMT, Alex Menkov  wrote:

>> The change fixes regression from JDK-8299414.
>> There is a deadlock between JvmtiVTMSTransitionDisabler and EscapeBarrier 
>> when virtual threads are in mount/unmount transition:
>> EscapeBarrier requests deoptimization which requires thread suspension.
>> JvmtiVTMSTransitionDisabler ctor waits until all in progress VTMS 
>> transitions complete, but they cannot be completed as thread is suspended.
>> To avoid the deadlock mount/unmount transitions should be completed before 
>> EscapeBarrier stuff.
>
> Alex Menkov has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - Merge branch 'openjdk:master' into follow_ref_deadlock
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - fix
>  - unproblem-list tests
>  - fix
>  - unproblem-list tests
>  - fix

Marked as reviewed by lmesnik (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14233#pullrequestreview-1454207021

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Alex Menkov]

> The change fixes regression from JDK-8299414.
> There is a deadlock between JvmtiVTMSTransitionDisabler and EscapeBarrier 
> when virtual threads are in mount/unmount transition:
> EscapeBarrier requests deoptimization which requires thread suspension.
> JvmtiVTMSTransitionDisabler ctor waits until all in progress VTMS transitions 
> complete, but they cannot be completed as thread is suspended.
> To avoid the deadlock mount/unmount transitions should be completed before 
> EscapeBarrier stuff.

Alex Menkov has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains eight additional commits since 
the last revision:

 - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
follow_ref_deadlock
 - Merge branch 'openjdk:master' into follow_ref_deadlock
 - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
follow_ref_deadlock
 - fix
 - unproblem-list tests
 - fix
 - unproblem-list tests
 - fix

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14233/files
  - new: https://git.openjdk.org/jdk/pull/14233/files/b2df4fd6..8c3ed263

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14233=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=14233=01-02

  Stats: 1647 lines in 164 files changed: 729 ins; 474 del; 444 mod
  Patch: https://git.openjdk.org/jdk/pull/14233.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14233/head:pull/14233

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

Integrated: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn

[Chris Plummer]

On Fri, 26 May 2023 03:21:12 GMT, Chris Plummer  wrote:

> Improve ThreadReference.ForceEarlyReturn to support virtual threads as long 
> as they are suspended and mounted.
> 
> [JDK-8308400](https://bugs.openjdk.org/browse/JDK-8308400) improved JVMTI 
> ForceEarlyReturn support for virtual threads. The spec now says it is 
> supported if the thread is suspended at an event (not a specific event as 
> previously spec'd), and additional support may also be provided. The 
> implementation now just requires that the virtual thread be mounted, so this 
> covers being suspended at an event, but also other situations in which the 
> virtual thread is mounted.
> 
> JDWP and JDI need to line up with JVMTI. Only spec changes will be needed.
> 
> A new test was added mostly to cover the unmounted virtual thread case that 
> results in OpaqueFrameException.
> 
> forceEarlyReturn002 test had previously been updated for virtual threads to 
> expect OpaqueFrameException. These changes have all been undone since the 
> test now runs the same when using virtual threads as with platform threads.

This pull request has now been integrated.

Changeset: 5531f6ba
Author:Chris Plummer 
URL:   
https://git.openjdk.org/jdk/commit/5531f6ba1b75f922f08501eda4b2a7a849ee49f5
Stats: 351 lines in 9 files changed: 302 ins; 29 del; 20 mod

8308819: add JDWP and JDI virtual thread support for 
ThreadReference.ForceEarlyReturn

Reviewed-by: sspitsyn, alanb

-

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

Re: RFR: 8308819: add JDWP and JDI virtual thread support for ThreadReference.ForceEarlyReturn [v2]

[Chris Plummer]

On Tue, 30 May 2023 19:32:11 GMT, Chris Plummer  wrote:

>> Improve ThreadReference.ForceEarlyReturn to support virtual threads as long 
>> as they are suspended and mounted.
>> 
>> [JDK-8308400](https://bugs.openjdk.org/browse/JDK-8308400) improved JVMTI 
>> ForceEarlyReturn support for virtual threads. The spec now says it is 
>> supported if the thread is suspended at an event (not a specific event as 
>> previously spec'd), and additional support may also be provided. The 
>> implementation now just requires that the virtual thread be mounted, so this 
>> covers being suspended at an event, but also other situations in which the 
>> virtual thread is mounted.
>> 
>> JDWP and JDI need to line up with JVMTI. Only spec changes will be needed.
>> 
>> A new test was added mostly to cover the unmounted virtual thread case that 
>> results in OpaqueFrameException.
>> 
>> forceEarlyReturn002 test had previously been updated for virtual threads to 
>> expect OpaqueFrameException. These changes have all been undone since the 
>> test now runs the same when using virtual threads as with platform threads.
>
> Chris Plummer has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Restore forceEarlyReturn() platform thread behaviour to only throw 
> NativeMethodException and get rid of assert. A future CR/CSR will resolve the 
> issue of OPAQUE_FRAME sometimes not meaning NativeMethodException for 
> platform threads.

Thanks for the reviews Alan and Serguei!

-

PR Comment: https://git.openjdk.org/jdk/pull/14167#issuecomment-1570932026

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v6]

[Chris Plummer]

On Mon, 29 May 2023 09:37:08 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 14 additional 
> commits since the last revision:
> 
>  - Allow for warning to be skipped when same agent loaded a second/subsequent 
> time
>  - Merge
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - Refresh package description
>  - Merge
>  - Tweak docs
>  - Merge
>  - Draft docs changes
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/43a8a7a4...a6d3c23c

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

> 229:if (agent->is_static_lib() && agent->is_loaded()) {
> 230:  return true;
> 231:}

This doesn't make sense to me. If  you pass in `null` for `os_lib`, then we 
return true if any loaded static lib is found. Is this an attempt to limit the 
warning to just the first static lib that is loaded? Also, why would `null` 
ever be passed in if there wasn't at least one static lib. Some clarify 
comments would be useful.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212282761

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Chris Plummer]

On Wed, 31 May 2023 15:01:37 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> Alan Bateman has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 17 additional 
> commits since the last revision:
> 
>  - Add impl note to document the XX option
>  - Cleanup
>  - Merge
>  - Allow for warning to be skipped when same agent loaded a second/subsequent 
> time
>  - Merge
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - Refresh package description
>  - Merge
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/92f81be3...2d9d5922

src/hotspot/share/prims/jvmti.xml line 653:

> 651: of agents in the live phase. This option suppresses the warning to 
> standard error when
> 652: starting an agent in the live phase.
> 653: 

Glad to see this section added. I was thinking something like this would be 
useful during my initial review, but didn't realize it was an option to 
document implementation specific details like this.

src/hotspot/share/prims/jvmtiAgent.cpp line 512:

> 510: 
> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the 
> command line
> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && 
> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {

While looking at some code related to this, I noticed a couple of typos in the 
pre-existing load_agent_from_executable() comment. See lines 265 ("cant't") and 
268 (".&&"). Maybe you could clean them up.

src/hotspot/share/prims/jvmtiAgent.cpp line 512:

> 510: 
> 511:   // Print warning if EnableDynamicAgentLoading not enabled on the 
> command line
> 512:   if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && 
> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) {

The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. Isn't 
the library always already loaded by the time we get here (the assert below 
seems to imply that)? If so, wouldn't it already be in the list? If it's not in 
the list yet, perhaps a comment explaining why would be helpful here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212231226
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212252550
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212268282

Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]

[Phil Race]

On Fri, 26 May 2023 08:31:46 GMT, JoKern65  wrote:

>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk 
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small 
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the 
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   forgotton _

I am Ok with the client-libs changes here. I did not look at anything else.
So you'll need more approvals before its ready to push.

-

Marked as reviewed by prr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1454101589

Re: RFR: 8309159: Some minor comment and code cleanup in jdk/com/sun/jdi/PopFramesTest.java

[Chris Plummer]

On Wed, 31 May 2023 00:37:06 GMT, Chris Plummer  wrote:

> PopFramesTest.java is a new test. It could use a couple of minor code 
> cleanups, and there is one unused local variable that can be removed.

Thanks for the reviews Serguei and Leonid!

-

PR Comment: https://git.openjdk.org/jdk/pull/14237#issuecomment-1570639326

Integrated: 8309159: Some minor comment and code cleanup in jdk/com/sun/jdi/PopFramesTest.java

[Chris Plummer]

On Wed, 31 May 2023 00:37:06 GMT, Chris Plummer  wrote:

> PopFramesTest.java is a new test. It could use a couple of minor code 
> cleanups, and there is one unused local variable that can be removed.

This pull request has now been integrated.

Changeset: eae1f59d
Author:Chris Plummer 
URL:   
https://git.openjdk.org/jdk/commit/eae1f59da966f68c8e11547aec123741c1d21fef
Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod

8309159: Some minor comment and code cleanup in 
jdk/com/sun/jdi/PopFramesTest.java

Reviewed-by: sspitsyn, lmesnik

-

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

Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64

[Daniel D . Daugherty]

On Wed, 31 May 2023 16:40:54 GMT, Daniel D. Daugherty  
wrote:

> A couple of trivial ProblemListings:
> [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList 
> jdk/incubator/vector/Float64VectorTests.java on aarch64
> [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList 
> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java

This pull request has now been integrated.

Changeset: 45473ef2
Author:Daniel D. Daugherty 
URL:   
https://git.openjdk.org/jdk/commit/45473ef23520271954fa7196a5be588f88337aaf
Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod

8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64
8309231: ProblemList 
vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java

Reviewed-by: darcy

-

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

Re: Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64

[Daniel D . Daugherty]

On Wed, 31 May 2023 16:48:06 GMT, Joe Darcy  wrote:

>> A couple of trivial ProblemListings:
>> [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList 
>> jdk/incubator/vector/Float64VectorTests.java on aarch64
>> [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList 
>> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java
>
> Marked as reviewed by darcy (Reviewer).

@jddarcy - Thanks for the fast review!

-

PR Comment: https://git.openjdk.org/jdk/pull/14250#issuecomment-1570578322

Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64

[Daniel D . Daugherty]

A couple of trivial ProblemListings:
[JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList 
jdk/incubator/vector/Float64VectorTests.java on aarch64
[JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList 
vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java

-

Commit messages:
 - 8309231: ProblemList 
vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java
 - 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64

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

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

Re: Integrated: 8309230: ProblemList jdk/incubator/vector/Float64VectorTests.java on aarch64

[Joe Darcy]

On Wed, 31 May 2023 16:40:54 GMT, Daniel D. Daugherty  
wrote:

> A couple of trivial ProblemListings:
> [JDK-8309230](https://bugs.openjdk.org/browse/JDK-8309230) ProblemList 
> jdk/incubator/vector/Float64VectorTests.java on aarch64
> [JDK-8309231](https://bugs.openjdk.org/browse/JDK-8309231) ProblemList 
> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java

Marked as reviewed by darcy (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14250#pullrequestreview-1453713285

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Alan Bateman]

> This is the implementation for JEP 451. There are two parts to this:
> 
> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded into 
> a running VM. For JVM TI, the message is printed to stderr from 
> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
> includes an update to the JVM TI spec and API docs to require the warning.
> 
> 2. If running with -Djdk.instrument.traceUsage or 
> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
> a trace message and stack trace.
> 
> Testing: tier1-6

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 17 additional commits since the 
last revision:

 - Add impl note to document the XX option
 - Cleanup
 - Merge
 - Allow for warning to be skipped when same agent loaded a second/subsequent 
time
 - Merge
 - Tweak javadoc, update test to use more test infra
 - Merge
 - Merge
 - Refresh package description
 - Merge
 - ... and 7 more: https://git.openjdk.org/jdk/compare/945d3951...2d9d5922

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13899/files
  - new: https://git.openjdk.org/jdk/pull/13899/files/a6d3c23c..2d9d5922

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=13899=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=13899=05-06

  Stats: 5984 lines in 158 files changed: 4481 ins; 816 del; 687 mod
  Patch: https://git.openjdk.org/jdk/pull/13899.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13899/head:pull/13899

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

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v5]

[Kirk Pepperdine]

Hi Ron,

Thank you for the response. I’d read the JEP you’ve linked also.

From what I’ve been able to discern, this change will affect a small majority 
of our customers as it will change how they deploy observability and 
diagnostics. I’ve spoken to the some of the product owners and they feel that 
this represents a one-off pain point that they can adjust to. The principal CS 
that have to deal directly with helping customers deploy had a completely 
different view point on the subject. As I sift through the noise the filter I’m 
applying is that I’m generally more interested in easing customer concerns over 
engineering ones.

I’ve read the relevant JEPs several times and I believe I understand the 
arguments being forth. I’m sorry but I just don’t agree that on by default 
dynamic loading of an agent has any connection to better code integrity. As 
someone who had to sit through 2 days of meetings trying to get a call to 
System.gc() that was happening every 2 minutes resulting in system failures 
turned off by setting a flag I can tell you that getting an agent dynamically 
loaded into a runtime isn’t easy to being with. While this experience may seem 
extreme, it is more common than you’d imagine. As such it is my opinion that 
this JEP that seems to have more importance to engineers than end 
users/customers. More over, I feel that it is inflicting an unnecessary change 
to deployments that is adding to complexity rather than easing it.

Other than that, I don’t feel I have anything more of use to add to the 
conversation except thank you for answering my questions.

Kind regards,
Kirk


> On May 31, 2023, at 4:59 AM, Ron Pressler  wrote:
> 
> 
> 
>> On 28 May 2023, at 17:25, Kirk Pepperdine  wrote:
>> 
>> 
>> Call me dumb.. but… I would have the say that the most puzzling piece of 
>> this entire JEP/proposal is that I’m failing to make the connect between how 
>> an agent is loaded and how that strengthens integrity. I keep re-reading 
>> this JEP looking for clues but I keep bumping my head again… "Giving free 
>> reign to tools would imply giving free reign to libraries, which is 
>> tantamount to giving up on integrity by default.”. IMO, this is a false 
>> equivalency that is not supported by any other point in the document. IOWs, 
>> there is nothing in this document that is giving me a clue as to how turning 
>> off dynamic attach improves integrity when I can achieve the same effects 
>> with a direct attach.
> 
> You can’t find the answer to your question in 451 because we factored out the 
> motivation for 451 (and several other integrated and future JEPs) into the 
> informational JEP, https://openjdk.org/jeps/8305968, which you must read to 
> understand the motivation.
> 
> In short, the goal is integrity *by default*, which means that what we seek 
> not an end to “superpowers" but the explicit granting of superpowers on the 
> command line. The problem is not superpowers, but superpowers that *are not 
> explicitly acknowledged by the application*. Agents loaded at startup have 
> always been explicitly acknowledged by the application, while those loaded 
> after startup have not. The goal is to make them explicitly acknowledged, not 
> “turned off”, and then you get the same for both ways of loading agents: the 
> application explicitly grants superpowers in both situations. We want the 
> capabilities offered by agents, and we want the application to be able to 
> track them.
> 
>> What I do know is that turning off dynamic attach by default will cause 
>> grief to those that already having to cope with exceptionally complex 
>> deployment. I would argue that the complexity of these deployments have 
>> dramatically increased since 2017. Do we really want to add to that 
>> complexity or should we be refocusing on adding features that help to reduce 
>> that complexity.
> 
> As the informational JEP explains, not having integrity BY DEFAULT, causes 
> grief too. Do or don’t, someone gets grief. Given that the vast majority of 
> Java programs never require or want agents — attached dynamically or 
> otherwise — given that many current uses of dynamically loaded agents are 
> better served by agents loaded at startup, and given that we have adding a 
> command line flag on the one hand vs not having integrity by default that 
> makes it impossible for applications to know the “map” of their codebase (see 
> the informational JEP), we believe that, when integrated over the entire 
> ecosystem, more grief is caused by not restricting dynamically loaded agents 
> than by restricting it.
> 
> — Ron
> 

Re: RFR: 8308286 Fix clang warnings in linux code [v3]

[Weijun Wang]

On Wed, 31 May 2023 13:37:06 GMT, Artem Semenov  wrote:

>> When using the clang compiler to build OpenJDk on Linux, we encounter 
>> various "warnings as errors".
>> They can be fixed with small changes.
>
> Artem Semenov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update

src/java.security.jgss/share/native/libj2gss/gssapi.h line 47:

> 45: 
> 46: // Condition was copied from
> 47: // 
> Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h

In the current version of the file above, it's written as

#if defined(__APPLE__) && (defined(__ppc__) ||...

Is it better?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211757380

Re: RFR: 8308286 Fix clang warnings in linux code [v2]

[Artem Semenov]

On Tue, 30 May 2023 08:14:59 GMT, Alexey Ushakov  wrote:

>> Artem Semenov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update
>
> src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 655:
> 
>> 653:   // linker loaded it. We use "base diff" in read_lib_segments call 
>> below.
>> 654: 
>> 655: if (ps_pdread(ph, (psaddr_t) link_map_addr + 
>> LINK_MAP_ADDR_OFFSET,
> 
> Extra white spaces before 'if'

done

> src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 662:
> 
>> 660: 
>> 661:   // read address of the name
>> 662:   if (ps_pdread(ph, (psaddr_t) link_map_addr + 
>> LINK_MAP_NAME_OFFSET,
> 
> Extra white-spaces before 'if'

done

> src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 717:
> 
>> 715: 
>> 716: // read next link_map address
>> 717: if (ps_pdread(ph, (psaddr_t) link_map_addr + 
>> LINK_MAP_NEXT_OFFSET,
> 
> Extra white-spaces before 'if'

done

> src/jdk.jpackage/share/native/common/tstrings.cpp line 60:
> 
>> 58: #endif
>> 59: // With g++ this compiles only with '-std=gnu++0x' option
>> 60: ret = vsnprintf(&*fmtout.begin(), fmtout.size(), format, 
>> args);
> 
> Extra white-spaces before 'ret'

done

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211732345
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211734132
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211734711
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211735111

Re: RFR: 8308286 Fix clang warnings in linux code [v3]

[Artem Semenov]

On Sun, 28 May 2023 03:57:40 GMT, Kim Barrett  wrote:

>> Artem Semenov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update
>
> src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c line 1163:
> 
>> 1161: #if defined(__clang__)
>> 1162: #pragma clang diagnostic push
>> 1163: #pragma clang diagnostic ignored "-Wparentheses"
> 
> I think this warning is because of the several `if (init_result = ...)`?  
> Better would be to just add the extra parens.

done

> src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 575:
> 
>> 573: if (ps_pdread(ph, (char *)link_map_addr + LINK_MAP_LD_OFFSET,
>> 574:_ld, sizeof(uintptr_t)) != PS_OK) {
>> 575: #else
> 
> What problem is being "fixed" by these?  I'm dubious that this is the best 
> solution to whatever the problem
> is, but can't evaluate or suggest alternatives without knowing what it is.

done

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211733228
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1211732832

Re: RFR: 8308286 Fix clang warnings in linux code [v3]

[Artem Semenov]

> When using the clang compiler to build OpenJDk on Linux, we encounter various 
> "warnings as errors".
> They can be fixed with small changes.

Artem Semenov has updated the pull request incrementally with one additional 
commit since the last revision:

  update

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14033/files
  - new: https://git.openjdk.org/jdk/pull/14033/files/3f343c26..d5b70cb2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14033=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=14033=01-02

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

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

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[Jaroslav Bachorík]

On Wed, May 31, 2023 at 3:16 PM Dan Heidinga  wrote:

> On Wed, May 31, 2023 at 7:13 AM Jaroslav Bachorík <
> jaroslav.bacho...@datadoghq.com> wrote:
>
>> Dear Team,
>>
>> I've been investigating the unusual JVM crashes occurring in JVMTI calls
>> on a J9 JVM. During my investigation, I scrutinized the `jmethodID`
>> definition closely, available here: [jmethodID definition](
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID
>> ).
>>
>> To paraphrase, the definition suggests that `jmethodID` identifies a Java
>> method, initializer, or constructor. These identifiers, once returned by
>> JVM TI functions and events, can be safely stored. However, when the class
>> is unloaded, they become invalid, rendering them unsuitable for use.
>>
>
> The "typical" usage pattern is to create a JNI reference (local or global)
> to the relevant class and keep that reference for the duration of the
> jmethodID's life.  If the jmethodID needs to exist past the end of the
> local method, then it needs to be accompanied in some way by a global
> reference to the class.  This works great for JNI as you typically need to
> look up the class first before looking up methods.  JVMTI is more a
> challenge here as it's possible to get the jmethodID without already
> holding the class - GetMethodDeclaringClass can help here if the jmethodID
> is still valid.
>

Unfortunately, GetMethodDeclaringClass would crash if the jmethodID is not
valid any more, so it really can not be used safely as well.


>
>
>> My interpretation is that the JVMTI user should verify the validity of a
>> `jmethodID` value before using it to prevent potential crashes. Would you
>> agree with this interpretation?
>>
>
> Mostly.  I agree that using an invalid jmethodID will cause crashes but
> unfortunately, I'm not aware of any spec'd methods to verify its validity.
>
>
>>
>> This sounds like a sensible requirement, but its practical application
>> remains unclear. As far as I know, methods can be unloaded concurrently to
>> the native code executing JVMTI functions. This introduces a potential race
>> condition where the JVM unloads the methods during the check->use flow,
>> making it only a partial solution. To complicate matters further, no method
>> exists to confirm whether a `jmethodID` is valid.
>>
>> Theoretically, we could monitor the `CompiledMethodUnload` event to track
>> the validity state, creating a constantly expanding set of unloaded
>> `jmethodID` values or a bloom filter, if one does not care about few
>> potential false positives. This strategy, however, doesn't address the
>> potential race condition, and it could even exacerbate it due to possible
>> event delays. This delay might mistakenly validate a `jmethodID` value that
>> has already been unloaded, but for which the event hasn't been delivered
>> yet.
>>
>
> And CompileMethodUnloaded isn't the right event either as the jmethodID
> could still be valid.  If there was a ClassUnloaded event, and a mapping
> from class->jmethodID, it would be possible to invalidate the jmethodIDs
> but I'm not aware of any spec'd events that provide those details.
>

Yes, I see that there are more pieces missing here :(


>
>
>>
>> Honestly, I don't see a way to use `jmethodID` safely unless the code
>> using it suspends the entire JVM and doesn't resume until it's finished
>> with that `jmethodID`. Any other approach might lead to JVM crashes, as
>> we've observed with J9.
>>
>> Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure
>> that using jmethodIDs for unloaded methods doesn't crash the JVM and even
>> provides useful information. This observation has led me to question
>> whether the documentation aligns with the Hotspot implementation,
>> especially given that following closely the documentation appears to
>> increase the risk associated with the use of `jmethodID` values.
>>
>
> I did a pass through the spec after becoming aware of the crashes you were
> seeing on J9 and couldn't find a spec-compliant way to prevent the
> crashes.  It feels like there's some missing methods in the spec to make
> your use case safe.
>

Indeed. This is all even worse when one would use ASGCT as there is not
even a theoretical possibility to create a strong reference to a class
holding a particular jmethodID - doing so for every single collected frame
would be a performance nightmare, not even mentioning the fact that ASGCT
is used from a signal handler where any complex JVMTI operations are simply
not safe.

-JB-


>
> --Dan
>
>
>> I welcome your thoughts and perspectives on this matter.
>>
>> Best regards,
>>
>> Jaroslav
>>
>

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[Dan Heidinga]

On Wed, May 31, 2023 at 7:13 AM Jaroslav Bachorík <
jaroslav.bacho...@datadoghq.com> wrote:

> Dear Team,
>
> I've been investigating the unusual JVM crashes occurring in JVMTI calls
> on a J9 JVM. During my investigation, I scrutinized the `jmethodID`
> definition closely, available here: [jmethodID definition](
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID
> ).
>
> To paraphrase, the definition suggests that `jmethodID` identifies a Java
> method, initializer, or constructor. These identifiers, once returned by
> JVM TI functions and events, can be safely stored. However, when the class
> is unloaded, they become invalid, rendering them unsuitable for use.
>

The "typical" usage pattern is to create a JNI reference (local or global)
to the relevant class and keep that reference for the duration of the
jmethodID's life.  If the jmethodID needs to exist past the end of the
local method, then it needs to be accompanied in some way by a global
reference to the class.  This works great for JNI as you typically need to
look up the class first before looking up methods.  JVMTI is more a
challenge here as it's possible to get the jmethodID without already
holding the class - GetMethodDeclaringClass can help here if the jmethodID
is still valid.


> My interpretation is that the JVMTI user should verify the validity of a
> `jmethodID` value before using it to prevent potential crashes. Would you
> agree with this interpretation?
>

Mostly.  I agree that using an invalid jmethodID will cause crashes but
unfortunately, I'm not aware of any spec'd methods to verify its validity.


>
> This sounds like a sensible requirement, but its practical application
> remains unclear. As far as I know, methods can be unloaded concurrently to
> the native code executing JVMTI functions. This introduces a potential race
> condition where the JVM unloads the methods during the check->use flow,
> making it only a partial solution. To complicate matters further, no method
> exists to confirm whether a `jmethodID` is valid.
>
> Theoretically, we could monitor the `CompiledMethodUnload` event to track
> the validity state, creating a constantly expanding set of unloaded
> `jmethodID` values or a bloom filter, if one does not care about few
> potential false positives. This strategy, however, doesn't address the
> potential race condition, and it could even exacerbate it due to possible
> event delays. This delay might mistakenly validate a `jmethodID` value that
> has already been unloaded, but for which the event hasn't been delivered
> yet.
>

And CompileMethodUnloaded isn't the right event either as the jmethodID
could still be valid.  If there was a ClassUnloaded event, and a mapping
from class->jmethodID, it would be possible to invalidate the jmethodIDs
but I'm not aware of any spec'd events that provide those details.


>
> Honestly, I don't see a way to use `jmethodID` safely unless the code
> using it suspends the entire JVM and doesn't resume until it's finished
> with that `jmethodID`. Any other approach might lead to JVM crashes, as
> we've observed with J9.
>
> Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure
> that using jmethodIDs for unloaded methods doesn't crash the JVM and even
> provides useful information. This observation has led me to question
> whether the documentation aligns with the Hotspot implementation,
> especially given that following closely the documentation appears to
> increase the risk associated with the use of `jmethodID` values.
>

I did a pass through the spec after becoming aware of the crashes you were
seeing on J9 and couldn't find a spec-compliant way to prevent the
crashes.  It feels like there's some missing methods in the spec to make
your use case safe.

--Dan


> I welcome your thoughts and perspectives on this matter.
>
> Best regards,
>
> Jaroslav
>

Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]

[Severin Gehwolf]

On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf  wrote:

>> Please review these test changes which implement automatic testing of 
>> container resource updates without JVM restart. Note that this merely tests 
>> container detection code handling this case. It doesn't do anything special 
>> for the JVM itself, though it might make sense to add some sanity checks 
>> should we detect certain limits changing. In another PR, though.
>> 
>> As to the test design, it works similar to the shared temp tests: Interact 
>> between the two containers by virtue of a shared filesystem `/tmp` and 
>> creating marker files there in order to make them cooperate. Note that the 
>> new test needs `podman` version `4.3.0` and better (`4.5` is current).
>> 
>> Testing:
>>  - [x] GHA
>>  - [x] Linux x86_64 container tests on cg v1 and cg v2 system
>>  - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and 
>> `docker`)
>
> Severin Gehwolf has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8308090-tests-container-on-fly-updates
>  - Fix whitespace
>  - 8308090: Add container tests for on-the-fly resource quota updates

Anyone willing to review this?

-

PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1570090062

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v5]

[Ron Pressler]

> On 28 May 2023, at 17:25, Kirk Pepperdine  wrote:
> 
> 
> Call me dumb.. but… I would have the say that the most puzzling piece of this 
> entire JEP/proposal is that I’m failing to make the connect between how an 
> agent is loaded and how that strengthens integrity. I keep re-reading this 
> JEP looking for clues but I keep bumping my head again… "Giving free reign to 
> tools would imply giving free reign to libraries, which is tantamount to 
> giving up on integrity by default.”. IMO, this is a false equivalency that is 
> not supported by any other point in the document. IOWs, there is nothing in 
> this document that is giving me a clue as to how turning off dynamic attach 
> improves integrity when I can achieve the same effects with a direct attach.

You can’t find the answer to your question in 451 because we factored out the 
motivation for 451 (and several other integrated and future JEPs) into the 
informational JEP, https://openjdk.org/jeps/8305968, which you must read to 
understand the motivation.

In short, the goal is integrity *by default*, which means that what we seek not 
an end to “superpowers" but the explicit granting of superpowers on the command 
line. The problem is not superpowers, but superpowers that *are not explicitly 
acknowledged by the application*. Agents loaded at startup have always been 
explicitly acknowledged by the application, while those loaded after startup 
have not. The goal is to make them explicitly acknowledged, not “turned off”, 
and then you get the same for both ways of loading agents: the application 
explicitly grants superpowers in both situations. We want the capabilities 
offered by agents, and we want the application to be able to track them.

> What I do know is that turning off dynamic attach by default will cause grief 
> to those that already having to cope with exceptionally complex deployment. I 
> would argue that the complexity of these deployments have dramatically 
> increased since 2017. Do we really want to add to that complexity or should 
> we be refocusing on adding features that help to reduce that complexity.

As the informational JEP explains, not having integrity BY DEFAULT, causes 
grief too. Do or don’t, someone gets grief. Given that the vast majority of 
Java programs never require or want agents — attached dynamically or otherwise 
— given that many current uses of dynamically loaded agents are better served 
by agents loaded at startup, and given that we have adding a command line flag 
on the one hand vs not having integrity by default that makes it impossible for 
applications to know the “map” of their codebase (see the informational JEP), 
we believe that, when integrated over the entire ecosystem, more grief is 
caused by not restricting dynamically loaded agents than by restricting it.

— Ron

Clarifying jmethodID Usage and Potential JVM Crashes

[Jaroslav Bachorík]

Dear Team,

I've been investigating the unusual JVM crashes occurring in JVMTI calls on
a J9 JVM. During my investigation, I scrutinized the `jmethodID` definition
closely, available here: [jmethodID definition](
https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID).

To paraphrase, the definition suggests that `jmethodID` identifies a Java
method, initializer, or constructor. These identifiers, once returned by
JVM TI functions and events, can be safely stored. However, when the class
is unloaded, they become invalid, rendering them unsuitable for use.

My interpretation is that the JVMTI user should verify the validity of a
`jmethodID` value before using it to prevent potential crashes. Would you
agree with this interpretation?

This sounds like a sensible requirement, but its practical application
remains unclear. As far as I know, methods can be unloaded concurrently to
the native code executing JVMTI functions. This introduces a potential race
condition where the JVM unloads the methods during the check->use flow,
making it only a partial solution. To complicate matters further, no method
exists to confirm whether a `jmethodID` is valid.

Theoretically, we could monitor the `CompiledMethodUnload` event to track
the validity state, creating a constantly expanding set of unloaded
`jmethodID` values or a bloom filter, if one does not care about few
potential false positives. This strategy, however, doesn't address the
potential race condition, and it could even exacerbate it due to possible
event delays. This delay might mistakenly validate a `jmethodID` value that
has already been unloaded, but for which the event hasn't been delivered
yet.

Honestly, I don't see a way to use `jmethodID` safely unless the code using
it suspends the entire JVM and doesn't resume until it's finished with that
`jmethodID`. Any other approach might lead to JVM crashes, as we've
observed with J9.

Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure
that using jmethodIDs for unloaded methods doesn't crash the JVM and even
provides useful information. This observation has led me to question
whether the documentation aligns with the Hotspot implementation,
especially given that following closely the documentation appears to
increase the risk associated with the use of `jmethodID` values.

I welcome your thoughts and perspectives on this matter.

Best regards,

Jaroslav

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Alan Bateman]

On Wed, 31 May 2023 10:34:56 GMT, Serguei Spitsyn  wrote:

> I was also thinking if this check has to be done in one place.

Yes, maybe JvmtiAgentList::load_agent, JvmtiAgent::load, or 
invoke_Agent_OnAttach. As part of the change to emit a warning when agent code 
is loaded into a running VM, invoke_Agent_OnAttach needs to check if 
EnableDynamicAgentLoading has been set on the command line, so looking at this 
VM option in one place would be nice.

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1569985853

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Serguei Spitsyn]

On Wed, 31 May 2023 10:30:22 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   minor renaming in new test TestJcmdNoAgentLoad.java

Thank you for the comment.
I was also thinking if this check has to be done in one place.

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1569939180

Changing granularity of jvmtis DynamicCodeGenerated for interpreter

[Vladimir Kempik]

Hello
I’ve found a need to change the granularity of JVMTI’s DynamicCodeGenerated for 
interpreter’s generated code.

Currently it shows just one big interpreter area, probably because of this code:

 JvmtiExport::post_dynamic_code_generated("Interpreter",
 
AbstractInterpreter::code()->code_start(),
 
AbstractInterpreter::code()->code_end());

However for template interpreter it’s possible to generate such entry for every 
bytecode instruction code generated ( e.g. one for sipush, etc). Just like 
-XX:+PrintInterpreter does.

What would be the ideal place to insert detailed 
JvmtiExport::post_dynamic_code_generated() calls  then ?

Thanks in advance, Vladimir

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading

[Alan Bateman]

On Wed, 31 May 2023 10:08:46 GMT, Serguei Spitsyn  wrote:

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

For the "load" command implemented in attachLister, do you remember why 
EnableDynamicAgentLoading is checked in attach_listener_thread_entry? Looking 
at it now, I wonder why the check isn't in load_agent. The code in 
attach_listener_thread_entry shouldn't have to special-case commands.

-

PR Comment: https://git.openjdk.org/jdk/pull/14244#issuecomment-1569932102

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Serguei Spitsyn]

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  minor renaming in new test TestJcmdNoAgentLoad.java

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14244/files
  - new: https://git.openjdk.org/jdk/pull/14244/files/dbe0d37a..829a0e9a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14244=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=14244=00-01

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

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

RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading

[Serguei Spitsyn]

The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
allow deployment to choose whether to allow agents to be loaded/started in the 
VM. The VM option does the right thing for tools using the Attach API but jcmd 
JVMTI.agent_load was missed. This should be fixed to disallow loading JVMTI 
agents when the EnableDynamicAgentLoading is false.

Testing:
 - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
 - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

-

Commit messages:
 - 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading

Changes: https://git.openjdk.org/jdk/pull/14244/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14244=00
  Issue: https://bugs.openjdk.org/browse/JDK-8304438
  Stats: 78 lines in 2 files changed: 78 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/14244.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244

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

Integrated: 8309044: Replace NULL with nullptr, final sweep of hotspot code

[Johan Sjölen]

On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen  wrote:

> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes 
> I'd appreciate if this was considered trivial.

This pull request has now been integrated.

Changeset: 4f161616
Author:Johan Sjölen 
URL:   
https://git.openjdk.org/jdk/commit/4f16161607edbf69f423ced1d3c24f7af058d46b
Stats: 114 lines in 67 files changed: 0 ins; 0 del; 114 mod

8309044: Replace NULL with nullptr, final sweep of hotspot code

Reviewed-by: stefank, dholmes, kvn, amitkumar

-

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

Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Johan Sjölen]

On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen  wrote:

>> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes 
>> I'd appreciate if this was considered trivial.
>
> Johan Sjölen has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Align
>  - Suggestions

Right, seems like the Windows CI fails at fetching JTReg. I'm merging this, 
thank you all for the reviews.

-

PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1569815714

Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]

[JoKern65]

On Tue, 30 May 2023 21:44:17 GMT, JoKern65  wrote:

>> src/hotspot/share/runtime/javaThread.cpp line 115:
>> 
>>> 113: #include 
>>> 114: #endif
>>> 115: 
>> 
>> Could these conditionals be included in globalDefinitions_xlc.hpp instead?
>
> In principle the `#include ` could be included in 
> globalDefinitions_xlc.hpp. But alloca.h implements it with a
> `#undef  alloca`
> `#define alloca(size)   __builtin_alloca (size)`
> If this new global define does not introduce new trouble (even in future 
> coding) when it is seen in every source, we can go this way.
> Are there any obstacles from anyone else?

At least the whole jdk actually builds with `#include ` in 
globalDefinitions_xlc.hpp

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1211272616

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v2]

[David Holmes]

On Wed, 31 May 2023 01:18:12 GMT, Alex Menkov  wrote:

>> The change fixes regression from JDK-8299414.
>> There is a deadlock between JvmtiVTMSTransitionDisabler and EscapeBarrier 
>> when virtual threads are in mount/unmount transition:
>> EscapeBarrier requests deoptimization which requires thread suspension.
>> JvmtiVTMSTransitionDisabler ctor waits until all in progress VTMS 
>> transitions complete, but they cannot be completed as thread is suspended.
>> To avoid the deadlock mount/unmount transitions should be completed before 
>> EscapeBarrier stuff.
>
> Alex Menkov has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - fix
>  - unproblem-list tests
>  - fix

Please ensure tier 1-5 testing been carried out before this is integrated and 
re-enables all those tests! Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/14233#issuecomment-1569671743