Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Jiangli Zhou
+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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Calvin Cheung
Thanks for the review, Ioi and Jiangli.

Calvin

On 11/15/18, 10:56 AM, 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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
In reply to this post by Jiangli Zhou
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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
On 11/15/18 22:39, [hidden email] 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.
To make it more clear...
The error message does not help much for people who are not used to create and use archives.
Does it mean that the field _allow_archiving_with_java_agent is set only at the dumping phase,
but the
AllowArchivingWithJavaAgent has to be always passed?


Thanks,
Serguei

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Jiangli Zhou
In reply to this post by serguei.spitsyn@oracle.com

'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, [hidden email] 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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
In reply to this post by serguei.spitsyn@oracle.com
Hi Ioi,

Thank you for the clarifications!

But then I have one more question to the fix in webrev.

http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html
  // 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);
  
      if (on_load_entry != NULL) {
        // Invoke the Agent_OnLoad function
        jint err = (*on_load_entry)(&main_vm, agent->options(), NULL);

If !agent->is_instrument_lib() is passed then it will be rejected with the message:
  "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping"
 
which is incorrect.

Probably, the fix needs to be something like this:
    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());
+     }
+     // CDS dumping does not support native JVMTI agent
+     else if (DumpSharedSpaces && !agent->is_instrument_lib()) {
+       vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());
+     }

Thanks,
Serguei





On 11/16/18 10:18, Ioi Lam wrote:


On 11/16/18 9:45 AM, [hidden email] wrote:
Hi Calvin,

On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,

Thanks for taking a look.

I think the option name is correct but the comment for Threads::create_vm_init_agents() is incorrect as I believe it will start both Java native agents. Otherwise, my new tests won't work.

It works because there is the system JVMTI (native) agent library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and transitively java agents as well via the libinstrument.so.

Dan already asked the question about your original reasoning/intention.
If your intention is to control java agent only then the fix is wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name is misleading.


The intention of this change is:

Disallow all use of native and java agents by default. -- using agents during dump time will cause undesirable side effects and make the CDS archive unsuitable for production environments.

However, for testing CDS itself, we want to use the Java agent (to run arbitrary Java code during dump time, such as causing GC at specific times).

As a result, what we are doing is:

    + Always disallow non-Java agents.
    + Allow java agents only if AllowArchivingWithJavaAgent is specified.

I think we should clean up the bug title and description if necessary. However, the behavior of this patch is what we want. And I think the naming of the AllowArchivingWithJavaAgent option is correct (as least in the sense of "anything not explicitly allow are strictly forbidden" :-)

Thanks
- Ioi


Below is my understanding:

In thread.cpp:

3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }

init_agents_at_startup() checks if the _agentList is empty in arguments.hpp:

 static bool init_agents_at_startup()      { return !_agentList.is_empty(); }

In arguments.cpp, entries will be added to the _agentList during the parsing of the -agentLib, -agentPath, and -javaagent arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be populated via the add_init_agent() function. For the -javaagent, the _agentList will be populated via the add_instrument_agent() function.

So in create_vm_init_agents(), it will just loop through the _agentList and starts all the agents. There's a check for !agent->is_instrument_agent() at line 4092 which is for not allowing any JVMTI (native) agent to be run during dump time.

One more reply to Serguei's comment below...

Okay, thank you for the update!

Thanks,
Serguei


thanks,
Calvin

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, [hidden email] 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.
The assignment at line 212 is for populating the archive header during CDS dump time.
It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
The value retrieved from the archive header could be different from the run time setting.
I will add a comment.

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
In reply to this post by serguei.spitsyn@oracle.com
On 11/16/18 10:52, Daniel D. Daugherty wrote:
On 11/16/18 1:18 PM, Ioi Lam wrote:


On 11/16/18 9:45 AM, [hidden email] wrote:
Hi Calvin,

On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,

Thanks for taking a look.

I think the option name is correct but the comment for Threads::create_vm_init_agents() is incorrect as I believe it will start both Java native agents. Otherwise, my new tests won't work.

It works because there is the system JVMTI (native) agent library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and transitively java agents as well via the libinstrument.so.

Dan already asked the question about your original reasoning/intention.
If your intention is to control java agent only then the fix is wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name is misleading.


The intention of this change is:

Disallow all use of native and java agents by default. -- using agents during dump time will cause undesirable side effects and make the CDS archive unsuitable for production environments.

However, for testing CDS itself, we want to use the Java agent (to run arbitrary Java code during dump time, such as causing GC at specific times).

As a result, what we are doing is:

    + Always disallow non-Java agents.
    + Allow java agents only if AllowArchivingWithJavaAgent is specified.

I think we should clean up the bug title and description if necessary. However, the behavior of this patch is what we want. And I think the naming of the AllowArchivingWithJavaAgent option is correct (as least in the sense of "anything not explicitly allow are strictly forbidden" :-)

I'm not sure that a Java agent can exist without the presence of the
backing native agent. For JLI/JPLIS, you have both the Java agent part
and the native agent that supports the Java agent.

So if you're truly disallowing non-Java agents, I'm not sure how that
Java agent can run without the backing native agent. However, this area
may have evolved since I last worked on it...

The fix in webrev checks the condition:
  !agent->is_instrument_lib()


Thanks,
Serguei

Dan



Thanks
- Ioi


Below is my understanding:

In thread.cpp:

3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }

init_agents_at_startup() checks if the _agentList is empty in arguments.hpp:

 static bool init_agents_at_startup()      { return !_agentList.is_empty(); }

In arguments.cpp, entries will be added to the _agentList during the parsing of the -agentLib, -agentPath, and -javaagent arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be populated via the add_init_agent() function. For the -javaagent, the _agentList will be populated via the add_instrument_agent() function.

So in create_vm_init_agents(), it will just loop through the _agentList and starts all the agents. There's a check for !agent->is_instrument_agent() at line 4092 which is for not allowing any JVMTI (native) agent to be run during dump time.

One more reply to Serguei's comment below...

Okay, thank you for the update!

Thanks,
Serguei


thanks,
Calvin

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, [hidden email] 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.
The assignment at line 212 is for populating the archive header during CDS dump time.
It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
The value retrieved from the archive header could be different from the run time setting.
I will add a comment.

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Ioi Lam
In reply to this post by serguei.spitsyn@oracle.com


On 11/16/18 10:52 AM, Daniel D. Daugherty wrote:

> On 11/16/18 1:18 PM, Ioi Lam wrote:
>>
>>
>> On 11/16/18 9:45 AM, [hidden email] wrote:
>>> Hi Calvin,
>>>
>>> On 11/16/18 09:26, Calvin Cheung wrote:
>>>> Serguei, Dan,
>>>>
>>>> Thanks for taking a look.
>>>>
>>>> I think the option name is correct but the comment for
>>>> Threads::create_vm_init_agents() is incorrect as I believe it will
>>>> start both Java native agents. Otherwise, my new tests won't work.
>>>
>>> It works because there is the system JVMTI (native) agent library
>>> libinstrument.so that is supporting java agents.
>>> It means that new option first impacts native agents and
>>> transitively java agents as well via the libinstrument.so.
>>>
>>> Dan already asked the question about your original reasoning/intention.
>>> If your intention is to control java agent only then the fix is
>>> wrong as the real impact is much bigger.
>>> If you wanted to control all the agents then the new option name is
>>> misleading.
>>>
>>>
>> The intention of this change is:
>>
>> Disallow all use of native and java agents by default. -- using
>> agents during dump time will cause undesirable side effects and make
>> the CDS archive unsuitable for production environments.
>>
>> However, for testing CDS itself, we want to use the Java agent (to
>> run arbitrary Java code during dump time, such as causing GC at
>> specific times).
>>
>> As a result, what we are doing is:
>>
>>     + Always disallow non-Java agents.
>>     + Allow java agents only if AllowArchivingWithJavaAgent is
>> specified.
>>
>> I think we should clean up the bug title and description if
>> necessary. However, the behavior of this patch is what we want. And I
>> think the naming of the AllowArchivingWithJavaAgent option is correct
>> (as least in the sense of "anything not explicitly allow are strictly
>> forbidden" :-)
>
> I'm not sure that a Java agent can exist without the presence of the
> backing native agent. For JLI/JPLIS, you have both the Java agent part
> and the native agent that supports the Java agent.
>
> So if you're truly disallowing non-Java agents, I'm not sure how that
> Java agent can run without the backing native agent. However, this area
> may have evolved since I last worked on it...
>
> Dan
>

Hi Dan,

You're correct that Java agents are implemented on top of native agents.
However, Java agents expose a more restricted set of capabilities than
the native agents. Since our CDS test only needs to use the
instrumentation provided by Java agents, it's safer to allow only Java
agents.

I should have said "forbid the direct use of native agents via the
-agentlib and -agentpath command-line options".

Thanks
- Ioi

>
>>
>> Thanks
>> - Ioi
>>
>>
>>>> Below is my understanding:
>>>>
>>>> In thread.cpp:
>>>>
>>>> 3697   if (Arguments::init_agents_at_startup()) {
>>>> 3698     create_vm_init_agents();
>>>> 3699   }
>>>>
>>>> init_agents_at_startup() checks if the _agentList is empty in
>>>> arguments.hpp:
>>>>
>>>>  static bool init_agents_at_startup()      { return
>>>> !_agentList.is_empty(); }
>>>>
>>>> In arguments.cpp, entries will be added to the _agentList during
>>>> the parsing of the -agentLib, -agentPath, and -javaagent arguments.
>>>> See lines around 2463-2502.
>>>> For the -agentLib and -agentPath args, the _agentList will be
>>>> populated via the add_init_agent() function. For the -javaagent,
>>>> the _agentList will be populated via the add_instrument_agent()
>>>> function.
>>>>
>>>> So in create_vm_init_agents(), it will just loop through the
>>>> _agentList and starts all the agents. There's a check for
>>>> !agent->is_instrument_agent() at line 4092 which is for not
>>>> allowing any JVMTI (native) agent to be run during dump time.
>>>>
>>>> One more reply to Serguei's comment below...
>>>
>>> Okay, thank you for the update!
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> 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, [hidden email] 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.
>>>> The assignment at line 212 is for populating the archive header
>>>> during CDS dump time.
>>>>>> It is not clear from scratch how these two values can be
>>>>>> different at line 1350. At least, some comment explaining it is
>>>>>> needed.
>>>> The value retrieved from the archive header could be different from
>>>> the run time setting.
>>>> I will add a comment.
>>>>
>>>>>> 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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Daniel D. Daugherty
On 11/16/18 2:01 PM, Ioi Lam wrote:

>
>
> On 11/16/18 10:52 AM, Daniel D. Daugherty wrote:
>> On 11/16/18 1:18 PM, Ioi Lam wrote:
>>>
>>>
>>> On 11/16/18 9:45 AM, [hidden email] wrote:
>>>> Hi Calvin,
>>>>
>>>> On 11/16/18 09:26, Calvin Cheung wrote:
>>>>> Serguei, Dan,
>>>>>
>>>>> Thanks for taking a look.
>>>>>
>>>>> I think the option name is correct but the comment for
>>>>> Threads::create_vm_init_agents() is incorrect as I believe it will
>>>>> start both Java native agents. Otherwise, my new tests won't work.
>>>>
>>>> It works because there is the system JVMTI (native) agent library
>>>> libinstrument.so that is supporting java agents.
>>>> It means that new option first impacts native agents and
>>>> transitively java agents as well via the libinstrument.so.
>>>>
>>>> Dan already asked the question about your original
>>>> reasoning/intention.
>>>> If your intention is to control java agent only then the fix is
>>>> wrong as the real impact is much bigger.
>>>> If you wanted to control all the agents then the new option name is
>>>> misleading.
>>>>
>>>>
>>> The intention of this change is:
>>>
>>> Disallow all use of native and java agents by default. -- using
>>> agents during dump time will cause undesirable side effects and make
>>> the CDS archive unsuitable for production environments.
>>>
>>> However, for testing CDS itself, we want to use the Java agent (to
>>> run arbitrary Java code during dump time, such as causing GC at
>>> specific times).
>>>
>>> As a result, what we are doing is:
>>>
>>>     + Always disallow non-Java agents.
>>>     + Allow java agents only if AllowArchivingWithJavaAgent is
>>> specified.
>>>
>>> I think we should clean up the bug title and description if
>>> necessary. However, the behavior of this patch is what we want. And
>>> I think the naming of the AllowArchivingWithJavaAgent option is
>>> correct (as least in the sense of "anything not explicitly allow are
>>> strictly forbidden" :-)
>>
>> I'm not sure that a Java agent can exist without the presence of the
>> backing native agent. For JLI/JPLIS, you have both the Java agent part
>> and the native agent that supports the Java agent.
>>
>> So if you're truly disallowing non-Java agents, I'm not sure how that
>> Java agent can run without the backing native agent. However, this area
>> may have evolved since I last worked on it...
>>
>> Dan
>>
>
> Hi Dan,
>
> You're correct that Java agents are implemented on top of native
> agents. However, Java agents expose a more restricted set of
> capabilities than the native agents. Since our CDS test only needs to
> use the instrumentation provided by Java agents, it's safer to allow
> only Java agents.
>
> I should have said "forbid the direct use of native agents via the
> -agentlib and -agentpath command-line options".

I'm good with that! Thanks for bearing with me...

Dan


>
> Thanks
> - Ioi
>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>>> Below is my understanding:
>>>>>
>>>>> In thread.cpp:
>>>>>
>>>>> 3697   if (Arguments::init_agents_at_startup()) {
>>>>> 3698     create_vm_init_agents();
>>>>> 3699   }
>>>>>
>>>>> init_agents_at_startup() checks if the _agentList is empty in
>>>>> arguments.hpp:
>>>>>
>>>>>  static bool init_agents_at_startup()      { return
>>>>> !_agentList.is_empty(); }
>>>>>
>>>>> In arguments.cpp, entries will be added to the _agentList during
>>>>> the parsing of the -agentLib, -agentPath, and -javaagent
>>>>> arguments. See lines around 2463-2502.
>>>>> For the -agentLib and -agentPath args, the _agentList will be
>>>>> populated via the add_init_agent() function. For the -javaagent,
>>>>> the _agentList will be populated via the add_instrument_agent()
>>>>> function.
>>>>>
>>>>> So in create_vm_init_agents(), it will just loop through the
>>>>> _agentList and starts all the agents. There's a check for
>>>>> !agent->is_instrument_agent() at line 4092 which is for not
>>>>> allowing any JVMTI (native) agent to be run during dump time.
>>>>>
>>>>> One more reply to Serguei's comment below...
>>>>
>>>> Okay, thank you for the update!
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> 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, [hidden email] 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.
>>>>> The assignment at line 212 is for populating the archive header
>>>>> during CDS dump time.
>>>>>>> It is not clear from scratch how these two values can be
>>>>>>> different at line 1350. At least, some comment explaining it is
>>>>>>> needed.
>>>>> The value retrieved from the archive header could be different
>>>>> from the run time setting.
>>>>> I will add a comment.
>>>>>
>>>>>>> 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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
In reply to this post by serguei.spitsyn@oracle.com
On 11/16/18 10:41, [hidden email] wrote:
Hi Ioi,

Thank you for the clarifications!

But then I have one more question to the fix in webrev.

http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html
  // 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);
  
      if (on_load_entry != NULL) {
        // Invoke the Agent_OnLoad function
        jint err = (*on_load_entry)(&main_vm, agent->options(), NULL);

If !agent->is_instrument_lib() is passed then it will be rejected with the message:
  "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping"
 
which is incorrect.

Probably, the fix needs to be something like this:
    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());
+     }
+     // CDS dumping does not support native JVMTI agent
+     else if (DumpSharedSpaces && !agent->is_instrument_lib()) {
+       vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());
+     }
Sorry, somehow I've copy-pasted wrong fragment.
It has to be:
    for (agent = Arguments::agents(); agent != NULL; agent = agent->next()) {
+     // CDS dumping does not support native JVMTI agent
+     if (DumpSharedSpaces) {
+       if(!agent->is_instrument_lib()) {
+         vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());
+       }
+       else if (!AllowArchivingWithJavaAgent) {
+         vm_exit_during_cds_dumping(
+             "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");
+       }
+     }
And this fragment at the begin of the function needs to be removed:
+   if (DumpSharedSpaces && !AllowArchivingWithJavaAgent) {
+     vm_exit_during_cds_dumping(
+         "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");
+   }

Thanks,
Serguei

Thanks,
Serguei





On 11/16/18 10:18, Ioi Lam wrote:


On 11/16/18 9:45 AM, [hidden email] wrote:
Hi Calvin,

On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,

Thanks for taking a look.

I think the option name is correct but the comment for Threads::create_vm_init_agents() is incorrect as I believe it will start both Java native agents. Otherwise, my new tests won't work.

It works because there is the system JVMTI (native) agent library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and transitively java agents as well via the libinstrument.so.

Dan already asked the question about your original reasoning/intention.
If your intention is to control java agent only then the fix is wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name is misleading.


The intention of this change is:

Disallow all use of native and java agents by default. -- using agents during dump time will cause undesirable side effects and make the CDS archive unsuitable for production environments.

However, for testing CDS itself, we want to use the Java agent (to run arbitrary Java code during dump time, such as causing GC at specific times).

As a result, what we are doing is:

    + Always disallow non-Java agents.
    + Allow java agents only if AllowArchivingWithJavaAgent is specified.

I think we should clean up the bug title and description if necessary. However, the behavior of this patch is what we want. And I think the naming of the AllowArchivingWithJavaAgent option is correct (as least in the sense of "anything not explicitly allow are strictly forbidden" :-)

Thanks
- Ioi


Below is my understanding:

In thread.cpp:

3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }

init_agents_at_startup() checks if the _agentList is empty in arguments.hpp:

 static bool init_agents_at_startup()      { return !_agentList.is_empty(); }

In arguments.cpp, entries will be added to the _agentList during the parsing of the -agentLib, -agentPath, and -javaagent arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be populated via the add_init_agent() function. For the -javaagent, the _agentList will be populated via the add_instrument_agent() function.

So in create_vm_init_agents(), it will just loop through the _agentList and starts all the agents. There's a check for !agent->is_instrument_agent() at line 4092 which is for not allowing any JVMTI (native) agent to be run during dump time.

One more reply to Serguei's comment below...

Okay, thank you for the update!

Thanks,
Serguei


thanks,
Calvin

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, [hidden email] 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.
The assignment at line 212 is for populating the archive header during CDS dump time.
It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
The value retrieved from the archive header could be different from the run time setting.
I will add a comment.

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Calvin Cheung
Hi Serguei,

I've updated the webrev based on your suggestions:
    http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/

Files changed since last time:
thead.cpp - your suggestion below
filemap.cpp - added a comment
DumpWithJavaAgent.java - some @ tag cleanup
DumpWithJvmtiAgent.java - some @ tag cleanup and added another test scenario (-agentlib without AllowArchivingWithJavaAgent)

thanks,
Calvin

On 11/16/18, 11:16 AM, [hidden email] wrote:
On 11/16/18 10:41, [hidden email] wrote:
Hi Ioi,

Thank you for the clarifications!

But then I have one more question to the fix in webrev.

http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html
  // 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);
  
      if (on_load_entry != NULL) {
        // Invoke the Agent_OnLoad function
        jint err = (*on_load_entry)(&main_vm, agent->options(), NULL);

If !agent->is_instrument_lib() is passed then it will be rejected with the message:
  "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping"
 
which is incorrect.

Probably, the fix needs to be something like this:
    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());
+     }
+     // CDS dumping does not support native JVMTI agent
+     else if (DumpSharedSpaces && !agent->is_instrument_lib()) {
+       vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());
+     }
Sorry, somehow I've copy-pasted wrong fragment.
It has to be:
    for (agent = Arguments::agents(); agent != NULL; agent = agent->next()) {
+     // CDS dumping does not support native JVMTI agent
+     if (DumpSharedSpaces) {
+       if(!agent->is_instrument_lib()) {
+         vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());
+       }
+       else if (!AllowArchivingWithJavaAgent) {
+         vm_exit_during_cds_dumping(
+             "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");
+       }
+     }
And this fragment at the begin of the function needs to be removed:
+   if (DumpSharedSpaces && !AllowArchivingWithJavaAgent) {
+     vm_exit_during_cds_dumping(
+         "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");
+   }

Thanks,
Serguei

Thanks,
Serguei





On 11/16/18 10:18, Ioi Lam wrote:


On 11/16/18 9:45 AM, [hidden email] wrote:
Hi Calvin,

On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,

Thanks for taking a look.

I think the option name is correct but the comment for Threads::create_vm_init_agents() is incorrect as I believe it will start both Java native agents. Otherwise, my new tests won't work.

It works because there is the system JVMTI (native) agent library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and transitively java agents as well via the libinstrument.so.

Dan already asked the question about your original reasoning/intention.
If your intention is to control java agent only then the fix is wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name is misleading.


The intention of this change is:

Disallow all use of native and java agents by default. -- using agents during dump time will cause undesirable side effects and make the CDS archive unsuitable for production environments.

However, for testing CDS itself, we want to use the Java agent (to run arbitrary Java code during dump time, such as causing GC at specific times).

As a result, what we are doing is:

    + Always disallow non-Java agents.
    + Allow java agents only if AllowArchivingWithJavaAgent is specified.

I think we should clean up the bug title and description if necessary. However, the behavior of this patch is what we want. And I think the naming of the AllowArchivingWithJavaAgent option is correct (as least in the sense of "anything not explicitly allow are strictly forbidden" :-)

Thanks
- Ioi


Below is my understanding:

In thread.cpp:

3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }

init_agents_at_startup() checks if the _agentList is empty in arguments.hpp:

 static bool init_agents_at_startup()      { return !_agentList.is_empty(); }

In arguments.cpp, entries will be added to the _agentList during the parsing of the -agentLib, -agentPath, and -javaagent arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be populated via the add_init_agent() function. For the -javaagent, the _agentList will be populated via the add_instrument_agent() function.

So in create_vm_init_agents(), it will just loop through the _agentList and starts all the agents. There's a check for !agent->is_instrument_agent() at line 4092 which is for not allowing any JVMTI (native) agent to be run during dump time.

One more reply to Serguei's comment below...

Okay, thank you for the update!

Thanks,
Serguei


thanks,
Calvin

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, [hidden email] 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.
The assignment at line 212 is for populating the archive header during CDS dump time.
It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
The value retrieved from the archive header could be different from the run time setting.
I will add a comment.

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

serguei.spitsyn@oracle.com
Hi Calvin,

It looks good in general.

New comment does not help much:
1362   // Java agents are allowed during run time. Therefore, the following condition is not
1363   // checked: !_allow_archiving_with_java_agent && AllowArchivingWithJavaAgent
1364   if (_allow_archiving_with_java_agent && !AllowArchivingWithJavaAgent) {
1365     FileMapInfo::fail_continue("The setting of the AllowArchivingWithJavaAgent is different "
1366                                "from the setting in the shared archive.");
1367     return false;
1368   }
There is a need to mention that the the _allow_archiving_with_java_agent
was set at the shared archive dump time while the
AllowArchivingWithJavaAgent
is the option passed to the current run.

Thanks,
Serguei


On 11/16/18 12:09 PM, Calvin Cheung wrote:
Hi Serguei,

I've updated the webrev based on your suggestions:
    http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/

Files changed since last time:
thead.cpp - your suggestion below
filemap.cpp - added a comment
DumpWithJavaAgent.java - some @ tag cleanup
DumpWithJvmtiAgent.java - some @ tag cleanup and added another test scenario (-agentlib without AllowArchivingWithJavaAgent)

thanks,
Calvin

On 11/16/18, 11:16 AM, [hidden email] wrote:
On 11/16/18 10:41, [hidden email] wrote:
Hi Ioi,

Thank you for the clarifications!

But then I have one more question to the fix in webrev.

http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html
*   // 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);*
*   *
*       if (on_load_entry != NULL) {*
*         // Invoke the Agent_OnLoad function*
*         jint err = (*on_load_entry)(&main_vm, agent->options(), NULL);*

If !*agent->is_instrument_lib()* is passed then it will be rejected with the <a class="moz-txt-link-freetext" href="message:*">message:*
  "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping"*
which is incorrect.

Probably, the fix needs to be something like this:
*     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());*
*+     }*
*+     // CDS dumping does not support native JVMTI agent*
*+     else if (DumpSharedSpaces&&  !agent->is_instrument_lib()) {*
*+       vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());*
*+     }*
Sorry, somehow I've copy-pasted wrong fragment.
It has to be:
*     for (agent = Arguments::agents(); agent != NULL; agent = agent->next()) {*
*+     // CDS dumping does not support native JVMTI agent*
*+     if (DumpSharedSpaces) {
+       if(!agent->is_instrument_lib()) {*
*+         vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent->name());*
*+       }*
*+       else if (!AllowArchivingWithJavaAgent) {*
*+         vm_exit_during_cds_dumping(*
*+             "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");*
*+       }
+     }
*
And this fragment at the begin of the function needs to be removed:
*+   if (DumpSharedSpaces&&  !AllowArchivingWithJavaAgent) {*
*+     vm_exit_during_cds_dumping(*
*+         "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");*
*+   }*

Thanks,
Serguei

Thanks,
Serguei





On 11/16/18 10:18, Ioi Lam wrote:


On 11/16/18 9:45 AM, [hidden email] wrote:
Hi Calvin,

On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,

Thanks for taking a look.

I think the option name is correct but the comment for Threads::create_vm_init_agents() is incorrect as I believe it will start both Java native agents. Otherwise, my new tests won't work.

It works because there is the system JVMTI (native) agent library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and transitively java agents as well via the libinstrument.so.

Dan already asked the question about your original reasoning/intention.
If your intention is to control java agent only then the fix is wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name is misleading.


The intention of this change is:

Disallow all use of native and java agents by default. -- using agents during dump time will cause undesirable side effects and make the CDS archive unsuitable for production environments.

However, for testing CDS itself, we want to use the Java agent (to run arbitrary Java code during dump time, such as causing GC at specific times).

As a result, what we are doing is:

    + Always disallow non-Java agents.
    + Allow java agents only if AllowArchivingWithJavaAgent is specified.

I think we should clean up the bug title and description if necessary. However, the behavior of this patch is what we want. And I think the naming of the AllowArchivingWithJavaAgent option is correct (as least in the sense of "anything not explicitly allow are strictly forbidden" :-)

Thanks
- Ioi


Below is my understanding:

In thread.cpp:

3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }

init_agents_at_startup() checks if the _agentList is empty in arguments.hpp:

 static bool init_agents_at_startup()      { return !_agentList.is_empty(); }

In arguments.cpp, entries will be added to the _agentList during the parsing of the -agentLib, -agentPath, and -javaagent arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be populated via the add_init_agent() function. For the -javaagent, the _agentList will be populated via the add_instrument_agent() function.

So in create_vm_init_agents(), it will just loop through the _agentList and starts all the agents. There's a check for !agent->is_instrument_agent() at line 4092 which is for not allowing any JVMTI (native) agent to be run during dump time.

One more reply to Serguei's comment below...

Okay, thank you for the update!

Thanks,
Serguei


thanks,
Calvin

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, [hidden email] 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.
The assignment at line 212 is for populating the archive header during CDS dump time.
It is not clear from scratch how these two values can be different at line 1350. At least, some comment explaining it is needed.
The value retrieved from the archive header could be different from the run time setting.
I will add a comment.

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 | Threaded
Open this post in threaded view
|

Re: RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

Calvin Cheung
Thanks, Serguei!

Calvin

On 11/16/18, 3:22 PM, [hidden email] wrote:

> Hi Calvin,
>
> It looks good to me.
> No need in new webrev if you update this comment as below.
>
> Thanks,
> Serguei
>
>
> On 11/16/18 2:00 PM, Calvin Cheung wrote:
>>
>>
>> On 11/16/18, 1:04 PM, [hidden email] wrote:
>>> Hi Calvin,
>>>
>>> It looks good in general.
>>>
>>> New comment does not help much:
>>> 1362   // Java agents are allowed during run time. Therefore, the
>>> following condition is not
>>> 1363   // checked: !_allow_archiving_with_java_agent&&
>>> AllowArchivingWithJavaAgent
>>> 1364   if (_allow_archiving_with_java_agent&&
>>> !AllowArchivingWithJavaAgent) {
>>> 1365     FileMapInfo::fail_continue("The setting of the
>>> AllowArchivingWithJavaAgent is different "
>>> 1366                                "from the setting in the shared
>>> archive.");
>>> 1367     return false;
>>> 1368   }
>>> There is a need to mention that the the
>>> _allow_archiving_with_java_agent
>>> was set at the shared archive dump time while
>>> theAllowArchivingWithJavaAgent
>>> is the option passed to the current run.
>> How about the following comment?
>>
>>   // Java agents are allowed during run time. Therefore, the
>> following condition is not
>>   // checked: (!_allow_archiving_with_java_agent &&
>> AllowArchivingWithJavaAgent)
>>   // Note: _allow_archiving_with_java_agent is set in the shared
>> archive during dump time
>>   // while AllowArchivingWithJavaAgent is set during the current run.
>>
>> thanks,
>> Calvin
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 11/16/18 12:09 PM, Calvin Cheung wrote:
>>>> Hi Serguei,
>>>>
>>>> I've updated the webrev based on your suggestions:
>>>> http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
>>>>
>>>> Files changed since last time:
>>>> thead.cpp - your suggestion below
>>>> filemap.cpp - added a comment
>>>> DumpWithJavaAgent.java - some @ tag cleanup
>>>> DumpWithJvmtiAgent.java - some @ tag cleanup and added another test
>>>> scenario (-agentlib without AllowArchivingWithJavaAgent)
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 11/16/18, 11:16 AM, [hidden email] wrote:
>>>>> On 11/16/18 10:41, [hidden email] wrote:
>>>>>> Hi Ioi,
>>>>>>
>>>>>> Thank you for the clarifications!
>>>>>>
>>>>>> But then I have one more question to the fix in webrev.
>>>>>>
>>>>>> http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html 
>>>>>>
>>>>>> *   // 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);*
>>>>>> *   *
>>>>>> *       if (on_load_entry != NULL) {*
>>>>>> *         // Invoke the Agent_OnLoad function*
>>>>>> *         jint err = (*on_load_entry)(&main_vm, agent->options(),
>>>>>> NULL);*
>>>>>>
>>>>>> If !*agent->is_instrument_lib()* is passed then it will be
>>>>>> rejected with the message:*
>>>>>>   "Must enable AllowArchivingWithJavaAgent in order to run Java
>>>>>> agent during CDS dumping"*
>>>>>> which is incorrect.
>>>>>>
>>>>>> Probably, the fix needs to be something like this:
>>>>>> *     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());*
>>>>>> *+     }*
>>>>>> *+     // CDS dumping does not support native JVMTI agent*
>>>>>> *+     else if (DumpSharedSpaces&& !agent->is_instrument_lib()) {*
>>>>>> *+       vm_exit_during_cds_dumping("CDS dumping does not support
>>>>>> native JVMTI agent, name", agent->name());*
>>>>>> *+     }*
>>>>> Sorry, somehow I've copy-pasted wrong fragment.
>>>>> It has to be:
>>>>> *     for (agent = Arguments::agents(); agent != NULL; agent =
>>>>> agent->next()) {*
>>>>> *+     // CDS dumping does not support native JVMTI agent*
>>>>> *+     if (DumpSharedSpaces) {
>>>>> +       if(!agent->is_instrument_lib()) {*
>>>>> *+         vm_exit_during_cds_dumping("CDS dumping does not
>>>>> support native JVMTI agent, name", agent->name());*
>>>>> *+       }*
>>>>> *+       else if (!AllowArchivingWithJavaAgent) {*
>>>>> *+         vm_exit_during_cds_dumping(*
>>>>> *+             "Must enable AllowArchivingWithJavaAgent in order
>>>>> to run Java agent during CDS dumping");*
>>>>> *+       }
>>>>> +     }
>>>>> *
>>>>> And this fragment at the begin of the function needs to be removed:
>>>>> *+   if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
>>>>> *+     vm_exit_during_cds_dumping(*
>>>>> *+         "Must enable AllowArchivingWithJavaAgent in order to
>>>>> run Java agent during CDS dumping");*
>>>>> *+   }*
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/16/18 10:18, Ioi Lam wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 11/16/18 9:45 AM, [hidden email] wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>> On 11/16/18 09:26, Calvin Cheung wrote:
>>>>>>>>> Serguei, Dan,
>>>>>>>>>
>>>>>>>>> Thanks for taking a look.
>>>>>>>>>
>>>>>>>>> I think the option name is correct but the comment for
>>>>>>>>> Threads::create_vm_init_agents() is incorrect as I believe it
>>>>>>>>> will start both Java native agents. Otherwise, my new tests
>>>>>>>>> won't work.
>>>>>>>>
>>>>>>>> It works because there is the system JVMTI (native) agent
>>>>>>>> library libinstrument.so that is supporting java agents.
>>>>>>>> It means that new option first impacts native agents and
>>>>>>>> transitively java agents as well via the libinstrument.so.
>>>>>>>>
>>>>>>>> Dan already asked the question about your original
>>>>>>>> reasoning/intention.
>>>>>>>> If your intention is to control java agent only then the fix is
>>>>>>>> wrong as the real impact is much bigger.
>>>>>>>> If you wanted to control all the agents then the new option
>>>>>>>> name is misleading.
>>>>>>>>
>>>>>>>>
>>>>>>> The intention of this change is:
>>>>>>>
>>>>>>> Disallow all use of native and java agents by default. -- using
>>>>>>> agents during dump time will cause undesirable side effects and
>>>>>>> make the CDS archive unsuitable for production environments.
>>>>>>>
>>>>>>> However, for testing CDS itself, we want to use the Java agent
>>>>>>> (to run arbitrary Java code during dump time, such as causing GC
>>>>>>> at specific times).
>>>>>>>
>>>>>>> As a result, what we are doing is:
>>>>>>>
>>>>>>>     + Always disallow non-Java agents.
>>>>>>>     + Allow java agents only if AllowArchivingWithJavaAgent is
>>>>>>> specified.
>>>>>>>
>>>>>>> I think we should clean up the bug title and description if
>>>>>>> necessary. However, the behavior of this patch is what we want.
>>>>>>> And I think the naming of the AllowArchivingWithJavaAgent option
>>>>>>> is correct (as least in the sense of "anything not explicitly
>>>>>>> allow are strictly forbidden" :-)
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>>
>>>>>>>
>>>>>>>>> Below is my understanding:
>>>>>>>>>
>>>>>>>>> In thread.cpp:
>>>>>>>>>
>>>>>>>>> 3697   if (Arguments::init_agents_at_startup()) {
>>>>>>>>> 3698     create_vm_init_agents();
>>>>>>>>> 3699   }
>>>>>>>>>
>>>>>>>>> init_agents_at_startup() checks if the _agentList is empty in
>>>>>>>>> arguments.hpp:
>>>>>>>>>
>>>>>>>>>  static bool init_agents_at_startup()      { return
>>>>>>>>> !_agentList.is_empty(); }
>>>>>>>>>
>>>>>>>>> In arguments.cpp, entries will be added to the _agentList
>>>>>>>>> during the parsing of the -agentLib, -agentPath, and
>>>>>>>>> -javaagent arguments. See lines around 2463-2502.
>>>>>>>>> For the -agentLib and -agentPath args, the _agentList will be
>>>>>>>>> populated via the add_init_agent() function. For the
>>>>>>>>> -javaagent, the _agentList will be populated via the
>>>>>>>>> add_instrument_agent() function.
>>>>>>>>>
>>>>>>>>> So in create_vm_init_agents(), it will just loop through the
>>>>>>>>> _agentList and starts all the agents. There's a check for
>>>>>>>>> !agent->is_instrument_agent() at line 4092 which is for not
>>>>>>>>> allowing any JVMTI (native) agent to be run during dump time.
>>>>>>>>>
>>>>>>>>> One more reply to Serguei's comment below...
>>>>>>>>
>>>>>>>> Okay, thank you for the update!
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Calvin
>>>>>>>>>
>>>>>>>>> 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, [hidden email] 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.
>>>>>>>>> The assignment at line 212 is for populating the archive
>>>>>>>>> header during CDS dump time.
>>>>>>>>>>> It is not clear from scratch how these two values can be
>>>>>>>>>>> different at line 1350. At least, some comment explaining it
>>>>>>>>>>> is needed.
>>>>>>>>> The value retrieved from the archive header could be different
>>>>>>>>> from the run time setting.
>>>>>>>>> I will add a comment.
>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>