Discussion:
RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping
(too old to reply)
Jiangli Zhou
2018-11-15 18:56:59 UTC
Permalink
+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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Calvin Cheung
2018-11-15 22:50:24 UTC
Permalink
Thanks for the review, Ioi and Jiangli.

Calvin
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 06:39:50 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Calvin,<br>
<br>
It seems the option name AllowArchivingWithJavaAgent is not right.<br>
In fact, it is about JVMTI (native) agents:<br> <br> <pre><font color="black"><b> // Create agents for -agentlib: -agentpath: and converted -Xrun</b></font> <font color="black"><b> // Invokes Agent_OnLoad</b></font> <font color="black"><b> // Called very early -- before JavaThreads exist</b></font> <font color="black"><b> void Threads::create_vm_init_agents() {</b></font> <font color="blue"><b>+ if (DumpSharedSpaces &amp;&amp; !AllowArchivingWithJavaAgent) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping(</b></font> <font color="blue"><b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b></font> <font color="blue"><b>+ }</b></font> <font color="black"><b> extern struct JavaVM_ main_vm;</b></font> <font color="black"><b> AgentLibrary* agent;</b></font> <font color="black"><b> </b></font> <font color="black"><b> JvmtiExport::enter_onload_phase();</b></font> <font color="black"><b> </b></font> <font color="black"><b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b></font> <font color="blue"><b>+ // CDS dumping does not support native JVMTI agent</b></font> <font color="blue"><b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b></font>
<font color="blue"><b>+ }</b></font>
<font color="blue"><b>+ </b></font>
<font color="black"><b> OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);</b></font></pre>
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,<br>
and Agent_OnLoad is an entrypoint in native agent library.<br>
So, I'm thinking if it'd make sense to rename the option to
something like below:<br>
  AllowArchivingWithJVMTIAgent or<br>
  AllowArchivingWithNativeAgent or<br>
  AllowArchivingWithAgent<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html</a><br>
<pre><span class="new"> 212 _allow_archiving_with_java_agent = AllowArchivingWithJavaAgent;
...
</span>
<span class="new"><span class="new">1350 if (_allow_archiving_with_java_agent &amp;&amp; !AllowArchivingWithJavaAgent) {</span>
<span class="new">1351 FileMapInfo::fail_continue("The setting of the AllowArchivingWithJavaAgent is different "</span>
<span class="new">1352 "from the setting in the shared archive.");</span>
<span class="new">1353 return false;</span>
<span class="new">1354 }

</span>The </span><span class="new"><span class="new">_allow_archiving_with_java_agent</span> is initialized with the </span><span class="new"><span class="new">AllowArchivingWithJavaAgent</span></span> at line 212.
<span class="new">It is not clear from scratch how these two values can be different at line 1350.
At least, some comment explaining it is needed.</span> <span class="new"></span></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/15/18 10:56, Jiangli Zhou wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4cb6dc63-31da-f479-6a44-***@oracle.com">+1
<br>
<br>
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.
<br>
<br>
Thanks,
<br>
<br>
Jiangli
<br>
<br>
<br>
On 11/15/18 9:29 AM, Ioi Lam wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
The changes look good.
<br>
<br>
Thanks
<br>
<br>
- Ioi
<br>
<br>
On 11/14/18 4:40 PM, Calvin Cheung wrote:
<br>
<blockquote type="cite">bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8201375">https://bugs.openjdk.java.net/browse/JDK-8201375</a>
<br>
<br>
webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/">http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/</a>
<br>
<br>
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
(<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8213813">https://bugs.openjdk.java.net/browse/JDK-8213813</a>) for
details.
<br>
<br>
Testing: ran hs-tier{1,2,3} tests through mach5.
<br>
<br>
thanks,
<br>
Calvin
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
s***@oracle.com
2018-11-16 06:49:45 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 11/15/18 22:39,
<a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ddc9e562-171e-8658-9f5e-***@oracle.com">
<div class="moz-cite-prefix">Hi Calvin,<br>
<br>
It seems the option name AllowArchivingWithJavaAgent is not
right.<br>
In fact, it is about JVMTI (native) agents:<br> <br> <pre><b> // Create agents for -agentlib: -agentpath: and converted -Xrun</b> <b> // Invokes Agent_OnLoad</b> <b> // Called very early -- before JavaThreads exist</b> <b> void Threads::create_vm_init_agents() {</b> <b>+ if (DumpSharedSpaces &amp;&amp; !AllowArchivingWithJavaAgent) {</b> <b>+ vm_exit_during_cds_dumping(</b> <b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b> <b>+ }</b> <b> extern struct JavaVM_ main_vm;</b> <b> AgentLibrary* agent;</b> <b> </b> <b> JvmtiExport::enter_onload_phase();</b> <b> </b> <b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b> <b>+ // CDS dumping does not support native JVMTI agent</b> <b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b> <b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b>
<b>+ }</b>
<b>+ </b>
<b> OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);</b></pre>
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,<br>
and Agent_OnLoad is an entrypoint in native agent library.<br>
So, I'm thinking if it'd make sense to rename the option to
something like below:<br>
  AllowArchivingWithJVMTIAgent or<br>
  AllowArchivingWithNativeAgent or<br>
  AllowArchivingWithAgent<br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html</a><br>
<pre><span class="new"> 212 _allow_archiving_with_java_agent = AllowArchivingWithJavaAgent;
...
</span>
<span class="new"><span class="new">1350 if (_allow_archiving_with_java_agent &amp;&amp; !AllowArchivingWithJavaAgent) {</span>
<span class="new">1351 FileMapInfo::fail_continue("The setting of the AllowArchivingWithJavaAgent is different "</span>
<span class="new">1352 "from the setting in the shared archive.");</span>
<span class="new">1353 return false;</span>
<span class="new">1354 }

</span>The </span><span class="new"><span class="new">_allow_archiving_with_java_agent</span> is initialized with the </span><span class="new"><span class="new">AllowArchivingWithJavaAgent</span></span> at line 212.
<span class="new">It is not clear from scratch how these two values can be different at line 1350.
At least, some comment explaining it is needed.</span></pre>
</div>
</blockquote>
To make it more clear...<br>
The error message does not help much for people who are not used to
create and use archives.<br>
Does it mean that the <span class="new">field
_allow_archiving_with_java_agent is set only at the dumping phase,<br>
but the </span><span class="new"><span class="new">AllowArchivingWithJavaAgent
has to be always passed?<br>
<br>
<br>
Thanks,<br>
Serguei</span></span><span class="new"></span>
<blockquote type="cite"
cite="mid:ddc9e562-171e-8658-9f5e-***@oracle.com">
<div class="moz-cite-prefix">
<pre><span class="new">
</span></pre>
Thanks,<br>
Serguei<br>
<br>
<br>
On 11/15/18 10:56, Jiangli Zhou wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4cb6dc63-31da-f479-6a44-***@oracle.com">+1 <br>
<br>
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. <br>
<br>
Thanks, <br>
<br>
Jiangli <br>
<br>
<br>
On 11/15/18 9:29 AM, Ioi Lam wrote: <br>
<blockquote type="cite">Hi Calvin, <br>
<br>
The changes look good. <br>
<br>
Thanks <br>
<br>
- Ioi <br>
<br>
On 11/14/18 4:40 PM, Calvin Cheung wrote: <br>
<blockquote type="cite">bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8201375"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201375</a>
<br>
<br>
webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/</a>
<br>
<br>
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 (<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8213813"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213813</a>)
for details. <br>
<br>
Testing: ran hs-tier{1,2,3} tests through mach5. <br>
<br>
thanks, <br>
Calvin <br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
Daniel D. Daugherty
2018-11-16 14:08:38 UTC
Permalink
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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Calvin Cheung
2018-11-16 17:26:18 UTC
Permalink
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.

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...

thanks,
Calvin
Post by Daniel D. Daugherty
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
$ 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
-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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
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.
Post by Daniel D. Daugherty
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.
Post by Daniel D. Daugherty
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 17:45:53 UTC
Permalink
Hi Calvin,
Post by Calvin Cheung
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.
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
init_agents_at_startup() checks if the _agentList is empty in
 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
Post by Calvin Cheung
thanks,
Calvin
Post by Daniel D. Daugherty
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
$ 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
    -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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by Daniel D. Daugherty
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.
Post by Daniel D. Daugherty
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Ioi Lam
2018-11-16 18:18:29 UTC
Permalink
Hi Calvin,
Post by Calvin Cheung
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
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
init_agents_at_startup() checks if the _agentList is empty in
 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
Post by Calvin Cheung
thanks,
Calvin
Post by Daniel D. Daugherty
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
$ 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
    -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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by Daniel D. Daugherty
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.
Post by Daniel D. Daugherty
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 18:41:39 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Ioi,<br>
<br>
Thank you for the clarifications!<br>
<br>
But then I have one more question to the fix in webrev.<br> <br> <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html</a><br> <pre><font color="black"><b> // Create agents for -agentlib: -agentpath: and converted -Xrun</b></font> <font color="black"><b> // Invokes Agent_OnLoad</b></font> <font color="black"><b> // Called very early -- before JavaThreads exist</b></font> <font color="black"><b> void Threads::create_vm_init_agents() {</b></font> <font color="blue"><b>+ if (DumpSharedSpaces &amp;&amp; !AllowArchivingWithJavaAgent) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping(</b></font> <font color="blue"><b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b></font> <font color="blue"><b>+ }</b></font> <font color="black"><b> extern struct JavaVM_ main_vm;</b></font> <font color="black"><b> AgentLibrary* agent;</b></font> <font color="black"><b> </b></font> <font color="black"><b> JvmtiExport::enter_onload_phase();</b></font> <font color="black"><b> </b></font> <font color="black"><b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b></font> <font color="blue"><b>+ // CDS dumping does not support native JVMTI agent</b></font> <font color="blue"><b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b></font> <font color="blue"><b>+ }</b></font> <font color="blue"><b>+ </b></font> <font color="black"><b> OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);</b></font> <font color="black"><b> </b></font> <font color="black"><b> if (on_load_entry != NULL) {</b></font> <font color="black"><b> // Invoke the Agent_OnLoad function</b></font> <font color="black"><b> jint err = (*on_load_entry)(&amp;main_vm, agent-&gt;options(), NULL);</b></font></pre>
<br>
If !<font color="blue"><b>agent-&gt;is_instrument_lib()</b></font>
is passed then it will be rejected with the message:<font
color="blue"><b><br>
  "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping"</b></font> <br>
which is incorrect.<br>
<br>
Probably, the fix needs to be something like this:<br> <pre><font color="black"><b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b></font> <font color="blue"><b>+ // CDS dumping does not support native JVMTI agent</b></font> <font color="blue"><b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b></font> <font color="blue"><b>+ }</b></font> <font color="blue"><b>+ // CDS dumping does not support native JVMTI agent</b></font> <font color="blue"><b>+ else if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b></font> <font color="blue"><b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b></font>
<font color="blue"><b>+ }</b></font>
</pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
<br>
<br>
On 11/16/18 10:18, Ioi Lam wrote:<br>
</div>
<blockquote type="cite"
cite="mid:25737bd7-9333-480b-8ee6-***@oracle.com">
<br>
<br>
On 11/16/18 9:45 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
On 11/16/18 09:26, Calvin Cheung wrote:
<br>
<blockquote type="cite">Serguei, Dan,
<br>
<br>
Thanks for taking a look.
<br>
<br>
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.
<br>
</blockquote>
<br>
It works because there is the system JVMTI (native) agent
library libinstrument.so that is supporting java agents.
<br>
It means that new option first impacts native agents and
transitively java agents as well via the libinstrument.so.
<br>
<br>
Dan already asked the question about your original
reasoning/intention.
<br>
If your intention is to control java agent only then the fix is
wrong as the real impact is much bigger.
<br>
If you wanted to control all the agents then the new option name
is misleading.
<br>
<br>
<br>
</blockquote>
The intention of this change is:
<br>
<br>
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.
<br>
<br>
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).
<br>
<br>
As a result, what we are doing is:
<br>
<br>
    + Always disallow non-Java agents.
<br>
    + Allow java agents only if AllowArchivingWithJavaAgent is
specified.
<br>
<br>
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" :-)
<br>
<br>
Thanks
<br>
- Ioi
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Below is my understanding:
<br>
<br>
In thread.cpp:
<br>
<br>
3697   if (Arguments::init_agents_at_startup()) {
<br>
3698     create_vm_init_agents();
<br>
3699   }
<br>
<br>
init_agents_at_startup() checks if the _agentList is empty in
arguments.hpp:
<br>
<br>
 static bool init_agents_at_startup()      { return
!_agentList.is_empty(); }
<br>
<br>
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.
<br>
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.
<br>
<br>
So in create_vm_init_agents(), it will just loop through the
_agentList and starts all the agents. There's a check for
!agent-&gt;is_instrument_agent() at line 4092 which is for not
allowing any JVMTI (native) agent to be run during dump time.
<br>
<br>
One more reply to Serguei's comment below...
<br>
</blockquote>
<br>
Okay, thank you for the update!
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<blockquote type="cite">
<br>
thanks,
<br>
Calvin
<br>
<br>
On 11/16/18, 6:08 AM, Daniel D. Daugherty wrote: <br> <blockquote type="cite">&gt; The flags -agentlib and
-agentpath are to run JVMTI (native) agents,
<br>
&gt; and Agent_OnLoad is an entrypoint in native agent
library.
<br>
<br>
The '-agentlib' and '-agentpath' options are used to load
and execute
<br>
a native library via the Agent_OnLoad() entry point.
However, the
<br>
agent does not have to use JVM/TI (it could be pure JNI):
<br>
<br>
$ java -help
<br>
<br>
    -agentlib:&lt;libname&gt;[=&lt;options&gt;]
<br>
                  load native agent library &lt;libname&gt;,
e.g. -agentlib:hprof
<br>
                  see also, -agentlib:jdwp=help and
-agentlib:hprof=help
<br>
    -agentpath:&lt;pathname&gt;[=&lt;options&gt;]
<br>
                  load native agent library by full pathname
<br>
<br>
The '-javaagent' option is used to load and execute a Java
agent:
<br>
<br>
    -javaagent:&lt;jarpath&gt;[=&lt;options&gt;]
<br>
                  load Java programming language agent, see
java.lang.instrument
<br>
<br>
so I agree that the option is misnamed (a bit), but the
reasons
<br>
needed to be clarified.
<br>
<br>
Dan
<br>
<br>
<br>
On 11/16/18 1:39 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
It seems the option name AllowArchivingWithJavaAgent is
not right.
<br>
In fact, it is about JVMTI (native) agents:
<br>
<br>
*// Create agents for -agentlib: -agentpath: and converted
-Xrun*
<br>
*// Invokes Agent_OnLoad*
<br>
*// Called very early -- before JavaThreads exist*
<br>
*void Threads::create_vm_init_agents() {*
<br>
*+ if (DumpSharedSpaces &amp;&amp;
!AllowArchivingWithJavaAgent) {*
<br>
*+ vm_exit_during_cds_dumping(*
<br>
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");*
<br>
*+ }*
<br>
*extern struct JavaVM_ main_vm;*
<br>
*AgentLibrary* agent;*
<br>
**
<br>
*JvmtiExport::enter_onload_phase();*
<br>
**
<br>
*for (agent = Arguments::agents(); agent != NULL; agent =
agent-&gt;next()) {*
<br>
*+ // CDS dumping does not support native JVMTI agent*
<br>
*+ if (DumpSharedSpaces &amp;&amp;
!agent-&gt;is_instrument_lib()) {*
<br>
*+ vm_exit_during_cds_dumping("CDS dumping does not
support native JVMTI agent, name", agent-&gt;name());*
<br>
*+ }*
<br>
*+ *
<br>
*OnLoadEntry_t on_load_entry =
lookup_agent_on_load(agent);*
<br>
The flags -agentlib and -agentpath are to run JVMTI
(native) agents,
<br>
and Agent_OnLoad is an entrypoint in native agent library.
<br>
So, I'm thinking if it'd make sense to rename the option
to something like below:
<br>
  AllowArchivingWithJVMTIAgent or
<br>
  AllowArchivingWithNativeAgent or
<br>
  AllowArchivingWithAgent
<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html</a>
<br>
212 _allow_archiving_with_java_agent =
AllowArchivingWithJavaAgent; ...
<br>
1350 if (_allow_archiving_with_java_agent &amp;&amp;
!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.
<br>
</blockquote>
</blockquote>
The assignment at line 212 is for populating the archive
header during CDS dump time.
<br>
<blockquote type="cite">
<blockquote type="cite">It is not clear from scratch how
these two values can be different at line 1350. At least,
some comment explaining it is needed.
<br>
</blockquote>
</blockquote>
The value retrieved from the archive header could be different
from the run time setting.
<br>
I will add a comment.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Thanks,
<br>
Serguei
<br>
<br>
<br>
On 11/15/18 10:56, Jiangli Zhou wrote:
<br>
<blockquote type="cite">+1
<br>
<br>
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.
<br>
<br>
Thanks,
<br>
<br>
Jiangli
<br>
<br>
<br>
On 11/15/18 9:29 AM, Ioi Lam wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
The changes look good.
<br>
<br>
Thanks
<br>
<br>
- Ioi
<br>
<br>
On 11/14/18 4:40 PM, Calvin Cheung wrote:
<br>
<blockquote type="cite">bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8201375">https://bugs.openjdk.java.net/browse/JDK-8201375</a>
<br>
<br>
webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/">http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/</a>
<br>
<br>
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
(<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8213813">https://bugs.openjdk.java.net/browse/JDK-8213813</a>)
for details.
<br>
<br>
Testing: ran hs-tier{1,2,3} tests through mach5.
<br>
<br>
thanks,
<br>
Calvin
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
s***@oracle.com
2018-11-16 19:16:05 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 11/16/18 10:41,
<a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6a5f2f33-6342-cae3-d47e-***@oracle.com">
<div class="moz-cite-prefix">Hi Ioi,<br>
<br>
Thank you for the clarifications!<br>
<br>
But then I have one more question to the fix in webrev.<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html</a><br> <pre><b> // Create agents for -agentlib: -agentpath: and converted -Xrun</b> <b> // Invokes Agent_OnLoad</b> <b> // Called very early -- before JavaThreads exist</b> <b> void Threads::create_vm_init_agents() {</b> <b>+ if (DumpSharedSpaces &amp;&amp; !AllowArchivingWithJavaAgent) {</b> <b>+ vm_exit_during_cds_dumping(</b> <b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b> <b>+ }</b> <b> extern struct JavaVM_ main_vm;</b> <b> AgentLibrary* agent;</b> <b> </b> <b> JvmtiExport::enter_onload_phase();</b> <b> </b> <b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b> <b>+ // CDS dumping does not support native JVMTI agent</b> <b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b> <b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b> <b>+ }</b> <b>+ </b> <b> OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);</b> <b> </b> <b> if (on_load_entry != NULL) {</b> <b> // Invoke the Agent_OnLoad function</b> <b> jint err = (*on_load_entry)(&amp;main_vm, agent-&gt;options(), NULL);</b></pre>
<br>
If !<b>agent-&gt;is_instrument_lib()</b> is passed then it will
be rejected with the message:<b><br>
  "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping"</b> <br>
which is incorrect.<br>
<br>
Probably, the fix needs to be something like this:<br> <pre><b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b> <b>+ // CDS dumping does not support native JVMTI agent</b> <b>+ if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b> <b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b> <b>+ }</b> <b>+ // CDS dumping does not support native JVMTI agent</b> <b>+ else if (DumpSharedSpaces &amp;&amp; !agent-&gt;is_instrument_lib()) {</b> <b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b>
<b>+ }</b>
</pre>
</div>
</blockquote>
Sorry, somehow I've copy-pasted wrong fragment.<br>
It has to be:<br> <pre><b> for (agent = Arguments::agents(); agent != NULL; agent = agent-&gt;next()) {</b>
<b>+ // CDS dumping does not support native JVMTI agent</b>
<b>+ if (DumpSharedSpaces) {
+ if(!agent-&gt;is_instrument_lib()) {</b> <b>+ vm_exit_during_cds_dumping("CDS dumping does not support native JVMTI agent, name", agent-&gt;name());</b>
<b>+ }</b>
<b>+ else if (!AllowArchivingWithJavaAgent) {</b>
<b>+ vm_exit_during_cds_dumping(</b>
<b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b>
<b>+ }
+ }
</b></pre>
And this fragment at the begin of the function needs to be removed:<br>
<pre><b>+ if (DumpSharedSpaces &amp;&amp; !AllowArchivingWithJavaAgent) {</b>
<b>+ vm_exit_during_cds_dumping(</b>
<b>+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");</b>
<b>+ }</b></pre>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:6a5f2f33-6342-cae3-d47e-***@oracle.com">
<div class="moz-cite-prefix"> Thanks,<br>
Serguei<br>
<br>
<br>
<br>
<br>
<br>
On 11/16/18 10:18, Ioi Lam wrote:<br>
</div>
<blockquote type="cite"
cite="mid:25737bd7-9333-480b-8ee6-***@oracle.com"> <br>
<br>
On 11/16/18 9:45 AM, <a class="moz-txt-link-abbreviated"
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> wrote: <br>
<blockquote type="cite">Hi Calvin, <br>
<br>
On 11/16/18 09:26, Calvin Cheung wrote: <br>
<blockquote type="cite">Serguei, Dan, <br>
<br>
Thanks for taking a look. <br>
<br>
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. <br>
</blockquote>
<br>
It works because there is the system JVMTI (native) agent
library libinstrument.so that is supporting java agents. <br>
It means that new option first impacts native agents and
transitively java agents as well via the libinstrument.so. <br>
<br>
Dan already asked the question about your original
reasoning/intention. <br>
If your intention is to control java agent only then the fix
is wrong as the real impact is much bigger. <br>
If you wanted to control all the agents then the new option
name is misleading. <br>
<br>
<br>
</blockquote>
The intention of this change is: <br>
<br>
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. <br>
<br>
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). <br>
<br>
As a result, what we are doing is: <br>
<br>
    + Always disallow non-Java agents. <br>
    + Allow java agents only if AllowArchivingWithJavaAgent is
specified. <br>
<br>
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" :-) <br>
<br>
Thanks <br>
- Ioi <br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Below is my understanding: <br>
<br>
In thread.cpp: <br>
<br>
3697   if (Arguments::init_agents_at_startup()) { <br>
3698     create_vm_init_agents(); <br>
3699   } <br>
<br>
init_agents_at_startup() checks if the _agentList is empty
in arguments.hpp: <br>
<br>
 static bool init_agents_at_startup()      { return
!_agentList.is_empty(); } <br>
<br>
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. <br>
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. <br>
<br>
So in create_vm_init_agents(), it will just loop through the
_agentList and starts all the agents. There's a check for
!agent-&gt;is_instrument_agent() at line 4092 which is for
not allowing any JVMTI (native) agent to be run during dump
time. <br>
<br>
One more reply to Serguei's comment below... <br>
</blockquote>
<br>
Okay, thank you for the update! <br>
<br>
Thanks, <br>
Serguei <br>
<br>
<blockquote type="cite"> <br>
thanks, <br>
Calvin <br>
<br>
On 11/16/18, 6:08 AM, Daniel D. Daugherty wrote: <br> <blockquote type="cite">&gt; The flags -agentlib and
-agentpath are to run JVMTI (native) agents, <br>
&gt; and Agent_OnLoad is an entrypoint in native agent
library. <br>
<br>
The '-agentlib' and '-agentpath' options are used to load
and execute <br>
a native library via the Agent_OnLoad() entry point.
However, the <br>
agent does not have to use JVM/TI (it could be pure JNI):
<br>
<br>
$ java -help <br>
<br>
    -agentlib:&lt;libname&gt;[=&lt;options&gt;] <br>
                  load native agent library
&lt;libname&gt;, e.g. -agentlib:hprof <br>
                  see also, -agentlib:jdwp=help and
-agentlib:hprof=help <br>
    -agentpath:&lt;pathname&gt;[=&lt;options&gt;] <br>
                  load native agent library by full
pathname <br>
<br>
The '-javaagent' option is used to load and execute a Java
agent: <br>
<br>
    -javaagent:&lt;jarpath&gt;[=&lt;options&gt;] <br>
                  load Java programming language agent,
see java.lang.instrument <br>
<br>
so I agree that the option is misnamed (a bit), but the
reasons <br>
needed to be clarified. <br>
<br>
Dan <br>
<br>
<br>
On 11/16/18 1:39 AM, <a class="moz-txt-link-abbreviated"
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>
wrote: <br>
<blockquote type="cite">Hi Calvin, <br>
<br>
It seems the option name AllowArchivingWithJavaAgent is
not right. <br>
In fact, it is about JVMTI (native) agents: <br>
<br>
*// Create agents for -agentlib: -agentpath: and
converted -Xrun* <br>
*// Invokes Agent_OnLoad* <br>
*// Called very early -- before JavaThreads exist* <br>
*void Threads::create_vm_init_agents() {* <br>
*+ if (DumpSharedSpaces &amp;&amp;
!AllowArchivingWithJavaAgent) {* <br>
*+ vm_exit_during_cds_dumping(* <br>
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");* <br>
*+ }* <br>
*extern struct JavaVM_ main_vm;* <br>
*AgentLibrary* agent;* <br>
** <br>
*JvmtiExport::enter_onload_phase();* <br>
** <br>
*for (agent = Arguments::agents(); agent != NULL; agent
= agent-&gt;next()) {* <br>
*+ // CDS dumping does not support native JVMTI agent* <br>
*+ if (DumpSharedSpaces &amp;&amp;
!agent-&gt;is_instrument_lib()) {* <br>
*+ vm_exit_during_cds_dumping("CDS dumping does not
support native JVMTI agent, name", agent-&gt;name());* <br>
*+ }* <br>
*+ * <br>
*OnLoadEntry_t on_load_entry =
lookup_agent_on_load(agent);* <br>
The flags -agentlib and -agentpath are to run JVMTI
(native) agents, <br>
and Agent_OnLoad is an entrypoint in native agent
library. <br>
So, I'm thinking if it'd make sense to rename the option
to something like below: <br>
  AllowArchivingWithJVMTIAgent or <br>
  AllowArchivingWithNativeAgent or <br>
  AllowArchivingWithAgent <br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html</a>
<br>
212 _allow_archiving_with_java_agent =
AllowArchivingWithJavaAgent; ... <br>
1350 if (_allow_archiving_with_java_agent &amp;&amp;
!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. <br>
</blockquote>
</blockquote>
The assignment at line 212 is for populating the archive
header during CDS dump time. <br>
<blockquote type="cite">
<blockquote type="cite">It is not clear from scratch how
these two values can be different at line 1350. At
least, some comment explaining it is needed. <br>
</blockquote>
</blockquote>
The value retrieved from the archive header could be
different from the run time setting. <br>
I will add a comment. <br>
<br>
<blockquote type="cite">
<blockquote type="cite">Thanks, <br>
Serguei <br>
<br>
<br>
On 11/15/18 10:56, Jiangli Zhou wrote: <br>
<blockquote type="cite">+1 <br>
<br>
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. <br>
<br>
Thanks, <br>
<br>
Jiangli <br>
<br>
<br>
On 11/15/18 9:29 AM, Ioi Lam wrote: <br>
<blockquote type="cite">Hi Calvin, <br>
<br>
The changes look good. <br>
<br>
Thanks <br>
<br>
- Ioi <br>
<br>
On 11/14/18 4:40 PM, Calvin Cheung wrote: <br>
<blockquote type="cite">bug: <a
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8201375"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8201375</a>
<br>
<br>
webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/</a>
<br>
<br>
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 (<a
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8213813"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8213813</a>)
for details. <br>
<br>
Testing: ran hs-tier{1,2,3} tests through mach5. <br>
<br>
thanks, <br>
Calvin <br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
Calvin Cheung
2018-11-16 20:09:28 UTC
Permalink
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
Post by s***@oracle.com
Post by s***@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.
* 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.
* 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");*
*+ }
+ }
*
*+ if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to run Java agent during CDS dumping");*
*+ }*
Thanks,
Serguei
Post by s***@oracle.com
Thanks,
Serguei
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
+ 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
Post by Calvin Cheung
3697 if (Arguments::init_agents_at_startup()) {
3698 create_vm_init_agents();
3699 }
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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
-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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 21:04:17 UTC
Permalink
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.

Thanks,
Serguei
Post by Calvin Cheung
Hi Serguei,
    http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
thead.cpp - your suggestion below
filemap.cpp - added a comment
scenario (-agentlib without AllowArchivingWithJavaAgent)
thanks,
Calvin
Post by s***@oracle.com
Post by s***@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.
*     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.
*     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");*
*+       }
+     }
*
*+   if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+     vm_exit_during_cds_dumping(*
*+         "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping");*
*+   }*
Thanks,
Serguei
Post by s***@oracle.com
Thanks,
Serguei
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
    + 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
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
 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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
    -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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Calvin Cheung
2018-11-16 22:00:46 UTC
Permalink
Hi Calvin,
It looks good in general.
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
Post by Calvin Cheung
Hi Serguei,
http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
thead.cpp - your suggestion below
filemap.cpp - added a comment
scenario (-agentlib without AllowArchivingWithJavaAgent)
thanks,
Calvin
Post by s***@oracle.com
Post by s***@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.
* 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.
* 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");*
*+ }
+ }
*
*+ if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping");*
*+ }*
Thanks,
Serguei
Post by s***@oracle.com
Thanks,
Serguei
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
+ 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
Post by Calvin Cheung
3697 if (Arguments::init_agents_at_startup()) {
3698 create_vm_init_agents();
3699 }
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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
-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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 23:22:01 UTC
Permalink
Hi Calvin,

It looks good to me.
No need in new webrev if you update this comment as below.

Thanks,
Serguei
Post by Calvin Cheung
Hi Calvin,
It looks good in general.
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
Post by Calvin Cheung
Hi Serguei,
http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
thead.cpp - your suggestion below
filemap.cpp - added a comment
scenario (-agentlib without AllowArchivingWithJavaAgent)
thanks,
Calvin
Post by s***@oracle.com
Post by s***@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.
*     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.
*     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");*
*+       }
+     }
*
*+   if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+     vm_exit_during_cds_dumping(*
*+         "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping");*
*+   }*
Thanks,
Serguei
Post by s***@oracle.com
Thanks,
Serguei
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
    + 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
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
 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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
    -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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
bug: https://bugs.openjdk.java.net/browse/JDK-8201375
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
Calvin Cheung
2018-11-16 23:34:03 UTC
Permalink
Thanks, Serguei!

Calvin
Hi Calvin,
It looks good to me.
No need in new webrev if you update this comment as below.
Thanks,
Serguei
Post by Calvin Cheung
Hi Calvin,
It looks good in general.
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
Post by Calvin Cheung
Hi Serguei,
http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
thead.cpp - your suggestion below
filemap.cpp - added a comment
scenario (-agentlib without AllowArchivingWithJavaAgent)
thanks,
Calvin
Post by s***@oracle.com
Post by s***@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.
* 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.
* 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");*
*+ }
+ }
*
*+ if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");*
*+ }*
Thanks,
Serguei
Post by s***@oracle.com
Thanks,
Serguei
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
+ 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
Post by Calvin Cheung
3697 if (Arguments::init_agents_at_startup()) {
3698 create_vm_init_agents();
3699 }
init_agents_at_startup() checks if the _agentList is empty in
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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI
(native) agents,
Post by s***@oracle.com
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
$ 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
-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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
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.
Post by s***@oracle.com
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.
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
bug: https://bugs.openjdk.java.net/browse/JDK-8201375
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
Daniel D. Daugherty
2018-11-16 18:52:30 UTC
Permalink
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
    + 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
Post by Ioi Lam
Thanks
- Ioi
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
 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
Post by Calvin Cheung
thanks,
Calvin
Post by Daniel D. Daugherty
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
$ 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
    -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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by Daniel D. Daugherty
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.
Post by Daniel D. Daugherty
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
s***@oracle.com
2018-11-16 18:55:52 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 11/16/18 10:52, Daniel D. Daugherty
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c63f7caa-450f-6b6a-9a1a-***@oracle.com">On
11/16/18 1:18 PM, Ioi Lam wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 11/16/18 9:45 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
On 11/16/18 09:26, Calvin Cheung wrote:
<br>
<blockquote type="cite">Serguei, Dan,
<br>
<br>
Thanks for taking a look.
<br>
<br>
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.
<br>
</blockquote>
<br>
It works because there is the system JVMTI (native) agent
library libinstrument.so that is supporting java agents.
<br>
It means that new option first impacts native agents and
transitively java agents as well via the libinstrument.so.
<br>
<br>
Dan already asked the question about your original
reasoning/intention.
<br>
If your intention is to control java agent only then the fix
is wrong as the real impact is much bigger.
<br>
If you wanted to control all the agents then the new option
name is misleading.
<br>
<br>
<br>
</blockquote>
The intention of this change is:
<br>
<br>
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.
<br>
<br>
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).
<br>
<br>
As a result, what we are doing is:
<br>
<br>
    + Always disallow non-Java agents.
<br>
    + Allow java agents only if AllowArchivingWithJavaAgent is
specified.
<br>
<br>
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" :-)
<br>
</blockquote>
<br>
I'm not sure that a Java agent can exist without the presence of
the
<br>
backing native agent. For JLI/JPLIS, you have both the Java agent
part
<br>
and the native agent that supports the Java agent.
<br>
<br>
So if you're truly disallowing non-Java agents, I'm not sure how
that
<br>
Java agent can run without the backing native agent. However, this
area
<br>
may have evolved since I last worked on it...
<br>
</blockquote>
<br>
The fix in webrev checks the condition:<br>
  <font color="blue"><b>!agent-&gt;is_instrument_lib()</b></font><br>
<br>
<br>
Thanks,<br>
Serguei<br>
<blockquote type="cite"
cite="mid:c63f7caa-450f-6b6a-9a1a-***@oracle.com">
<br>
Dan
<br>
<br>
<br>
<blockquote type="cite">
<br>
Thanks
<br>
- Ioi
<br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Below is my understanding:
<br>
<br>
In thread.cpp:
<br>
<br>
3697   if (Arguments::init_agents_at_startup()) {
<br>
3698     create_vm_init_agents();
<br>
3699   }
<br>
<br>
init_agents_at_startup() checks if the _agentList is empty
in arguments.hpp:
<br>
<br>
 static bool init_agents_at_startup()      { return
!_agentList.is_empty(); }
<br>
<br>
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.
<br>
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.
<br>
<br>
So in create_vm_init_agents(), it will just loop through the
_agentList and starts all the agents. There's a check for
!agent-&gt;is_instrument_agent() at line 4092 which is for
not allowing any JVMTI (native) agent to be run during dump
time.
<br>
<br>
One more reply to Serguei's comment below...
<br>
</blockquote>
<br>
Okay, thank you for the update!
<br>
<br>
Thanks,
<br>
Serguei
<br>
<br>
<blockquote type="cite">
<br>
thanks,
<br>
Calvin
<br>
<br>
On 11/16/18, 6:08 AM, Daniel D. Daugherty wrote: <br> <blockquote type="cite">&gt; The flags -agentlib and
-agentpath are to run JVMTI (native) agents,
<br>
&gt; and Agent_OnLoad is an entrypoint in native agent
library.
<br>
<br>
The '-agentlib' and '-agentpath' options are used to load
and execute
<br>
a native library via the Agent_OnLoad() entry point.
However, the
<br>
agent does not have to use JVM/TI (it could be pure JNI):
<br>
<br>
$ java -help
<br>
<br>
    -agentlib:&lt;libname&gt;[=&lt;options&gt;]
<br>
                  load native agent library
&lt;libname&gt;, e.g. -agentlib:hprof
<br>
                  see also, -agentlib:jdwp=help and
-agentlib:hprof=help
<br>
    -agentpath:&lt;pathname&gt;[=&lt;options&gt;]
<br>
                  load native agent library by full
pathname
<br>
<br>
The '-javaagent' option is used to load and execute a Java
agent:
<br>
<br>
    -javaagent:&lt;jarpath&gt;[=&lt;options&gt;]
<br>
                  load Java programming language agent,
see java.lang.instrument
<br>
<br>
so I agree that the option is misnamed (a bit), but the
reasons
<br>
needed to be clarified.
<br>
<br>
Dan
<br>
<br>
<br>
On 11/16/18 1:39 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
It seems the option name AllowArchivingWithJavaAgent is
not right.
<br>
In fact, it is about JVMTI (native) agents:
<br>
<br>
*// Create agents for -agentlib: -agentpath: and
converted -Xrun*
<br>
*// Invokes Agent_OnLoad*
<br>
*// Called very early -- before JavaThreads exist*
<br>
*void Threads::create_vm_init_agents() {*
<br>
*+ if (DumpSharedSpaces &amp;&amp;
!AllowArchivingWithJavaAgent) {*
<br>
*+ vm_exit_during_cds_dumping(*
<br>
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");*
<br>
*+ }*
<br>
*extern struct JavaVM_ main_vm;*
<br>
*AgentLibrary* agent;*
<br>
**
<br>
*JvmtiExport::enter_onload_phase();*
<br>
**
<br>
*for (agent = Arguments::agents(); agent != NULL; agent
= agent-&gt;next()) {*
<br>
*+ // CDS dumping does not support native JVMTI agent*
<br>
*+ if (DumpSharedSpaces &amp;&amp;
!agent-&gt;is_instrument_lib()) {*
<br>
*+ vm_exit_during_cds_dumping("CDS dumping does not
support native JVMTI agent, name", agent-&gt;name());*
<br>
*+ }*
<br>
*+ *
<br>
*OnLoadEntry_t on_load_entry =
lookup_agent_on_load(agent);*
<br>
The flags -agentlib and -agentpath are to run JVMTI
(native) agents,
<br>
and Agent_OnLoad is an entrypoint in native agent
library.
<br>
So, I'm thinking if it'd make sense to rename the option
to something like below:
<br>
  AllowArchivingWithJVMTIAgent or
<br>
  AllowArchivingWithNativeAgent or
<br>
  AllowArchivingWithAgent
<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html">http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html</a>
<br>
212 _allow_archiving_with_java_agent =
AllowArchivingWithJavaAgent; ...
<br>
1350 if (_allow_archiving_with_java_agent &amp;&amp;
!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.
<br>
</blockquote>
</blockquote>
The assignment at line 212 is for populating the archive
header during CDS dump time.
<br>
<blockquote type="cite">
<blockquote type="cite">It is not clear from scratch how
these two values can be different at line 1350. At
least, some comment explaining it is needed.
<br>
</blockquote>
</blockquote>
The value retrieved from the archive header could be
different from the run time setting.
<br>
I will add a comment.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">Thanks,
<br>
Serguei
<br>
<br>
<br>
On 11/15/18 10:56, Jiangli Zhou wrote:
<br>
<blockquote type="cite">+1
<br>
<br>
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.
<br>
<br>
Thanks,
<br>
<br>
Jiangli
<br>
<br>
<br>
On 11/15/18 9:29 AM, Ioi Lam wrote:
<br>
<blockquote type="cite">Hi Calvin,
<br>
<br>
The changes look good.
<br>
<br>
Thanks
<br>
<br>
- Ioi
<br>
<br>
On 11/14/18 4:40 PM, Calvin Cheung wrote:
<br>
<blockquote type="cite">bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8201375">https://bugs.openjdk.java.net/browse/JDK-8201375</a>
<br>
<br>
webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/">http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/</a>
<br>
<br>
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
(<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8213813">https://bugs.openjdk.java.net/browse/JDK-8213813</a>)
for details.
<br>
<br>
Testing: ran hs-tier{1,2,3} tests through mach5.
<br>
<br>
thanks,
<br>
Calvin
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
Ioi Lam
2018-11-16 19:01:59 UTC
Permalink
Post by Daniel D. Daugherty
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
    + 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
Post by Daniel D. Daugherty
Post by Ioi Lam
Thanks
- Ioi
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
 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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
    -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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Daniel D. Daugherty
2018-11-16 19:03:53 UTC
Permalink
Post by Ioi Lam
Post by Daniel D. Daugherty
Post by Ioi Lam
Hi Calvin,
Post by Calvin Cheung
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.
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).
    + 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
Post by Ioi Lam
Thanks
- Ioi
Post by Daniel D. Daugherty
Post by Ioi Lam
Thanks
- Ioi
Post by Calvin Cheung
3697   if (Arguments::init_agents_at_startup()) {
3698     create_vm_init_agents();
3699   }
 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
Post by Calvin Cheung
thanks,
Calvin
Post by s***@oracle.com
Post by s***@oracle.com
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
Post by s***@oracle.com
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
$ 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
    -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
Post by s***@oracle.com
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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.
Post by s***@oracle.com
Post by s***@oracle.com
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.
Post by s***@oracle.com
Post by s***@oracle.com
Thanks,
Serguei
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Jiangli Zhou
2018-11-16 18:03:27 UTC
Permalink
'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
Post by Daniel D. Daugherty
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
$ 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
    -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
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not right.
*// 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
  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
Post by 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
Hi Calvin,
The changes look good.
Thanks
- Ioi
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
Loading...