On Wed, 31 May 2023 15:01:37 GMT, Alan Bateman <al...@openjdk.org> 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: <p/> 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
