'AllowArchivingWithAgent' is also one of the earlier suggested naming (I voted for it :)). I think it servers the purpose better. We mainly want a diagnosis flag that allows us to use a JVMTI agent at CDS dump time for testing purpose only. Even for the 'allowed' usage, it may not have a complete support for all cases (so a diagnosis flag is needed to turn on the mode).

Thanks,

Jiangli


On 11/16/18 6:08 AM, Daniel D. Daugherty wrote:
> The flags -agentlib and -agentpath are to run JVMTI (native) agents,
> and Agent_OnLoad is an entrypoint in native agent library.

The '-agentlib' and '-agentpath' options are used to load and execute
a native library via the Agent_OnLoad() entry point. However, the
agent does not have to use JVM/TI (it could be pure JNI):

$ java -help

    -agentlib:<libname>[=<options>]
                  load native agent library <libname>, e.g. -agentlib:hprof
                  see also, -agentlib:jdwp=help and -agentlib:hprof=help
    -agentpath:<pathname>[=<options>]
                  load native agent library by full pathname

The '-javaagent' option is used to load and execute a Java agent:

    -javaagent:<jarpath>[=<options>]
                  load Java programming language agent, see java.lang.instrument

so I agree that the option is misnamed (a bit), but the reasons
needed to be clarified.

Dan


On 11/16/18 1:39 AM, serguei.spit...@oracle.com wrote:
Hi Calvin,

It seems the option name AllowArchivingWithJavaAgent is not right.
In fact, it is about JVMTI (native) agents:

*// Create agents for -agentlib: -agentpath: and converted -Xrun*
*// Invokes Agent_OnLoad*
*// Called very early -- before JavaThreads exist*
*void Threads::create_vm_init_agents() {*
*+ if (DumpSharedSpaces && !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");*
*+ }*
*extern struct JavaVM_ main_vm;*
*AgentLibrary* agent;*
**
*JvmtiExport::enter_onload_phase();*
**
*for (agent = Arguments::agents(); agent != NULL; agent = agent->next()) {*
*+ // CDS dumping does not support native JVMTI agent*
*+ if (DumpSharedSpaces && !agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());*
*+ }*
*+ *
*OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);*
The flags -agentlib and -agentpath are to run JVMTI (native) agents,
and Agent_OnLoad is an entrypoint in native agent library.
So, I'm thinking if it'd make sense to rename the option to something like below:
  AllowArchivingWithJVMTIAgent or
  AllowArchivingWithNativeAgent or
  AllowArchivingWithAgent


http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html
212 _allow_archiving_with_java_agent = AllowArchivingWithJavaAgent; ...
1350 if (_allow_archiving_with_java_agent && !AllowArchivingWithJavaAgent) { 1351 FileMapInfo::fail_continue("The setting of the AllowArchivingWithJavaAgent is different " 1352 "from the setting in the shared archive."); 1353 return false; 1354 } The _allow_archiving_with_java_agent is initialized with the AllowArchivingWithJavaAgent at line 212. It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
Thanks,
Serguei


On 11/15/18 10:56, Jiangli Zhou wrote:
+1

Adding serviceability-dev mailing list. It would be good to have this reviewed by the JVMTI experts also to make sure all cases are covered.

Thanks,

Jiangli


On 11/15/18 9:29 AM, Ioi Lam wrote:
Hi Calvin,

The changes look good.

Thanks

- Ioi

On 11/14/18 4:40 PM, Calvin Cheung wrote:
bug: https://bugs.openjdk.java.net/browse/JDK-8201375

webrev: http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/

This change is for adding a diagnostic vm option (AllowArchivingWithJavaAgent) to allow the -javaagent in the command line during CDS dumping. Please refer to the corresponding CSR (https://bugs.openjdk.java.net/browse/JDK-8213813) for details.

Testing: ran hs-tier{1,2,3} tests through mach5.

thanks,
Calvin




Reply via email to