Hi Calvin,
It looks good to me.
No need in new webrev if you update this comment as below.
Thanks,
Serguei
On 11/16/18 2:00 PM, Calvin Cheung wrote:
On 11/16/18, 1:04 PM, serguei.spit...@oracle.com wrote:
Hi Calvin,
It looks good in general.
New comment does not help much:
1362 // Java agents are allowed during run time. Therefore, the
following condition is not
1363 // checked: !_allow_archiving_with_java_agent&&
AllowArchivingWithJavaAgent
1364 if (_allow_archiving_with_java_agent&&
!AllowArchivingWithJavaAgent) {
1365 FileMapInfo::fail_continue("The setting of the
AllowArchivingWithJavaAgent is different "
1366 "from the setting in the shared
archive.");
1367 return false;
1368 }
There is a need to mention that the the _allow_archiving_with_java_agent
was set at the shared archive dump time while
theAllowArchivingWithJavaAgent
is the option passed to the current run.
How about the following comment?
// Java agents are allowed during run time. Therefore, the following
condition is not
// checked: (!_allow_archiving_with_java_agent &&
AllowArchivingWithJavaAgent)
// Note: _allow_archiving_with_java_agent is set in the shared
archive during dump time
// while AllowArchivingWithJavaAgent is set during the current run.
thanks,
Calvin
Thanks,
Serguei
On 11/16/18 12:09 PM, Calvin Cheung wrote:
Hi Serguei,
I've updated the webrev based on your suggestions:
http://cr.openjdk.java.net/~ccheung/8201375/webrev.01/
Files changed since last time:
thead.cpp - your suggestion below
filemap.cpp - added a comment
DumpWithJavaAgent.java - some @ tag cleanup
DumpWithJvmtiAgent.java - some @ tag cleanup and added another test
scenario (-agentlib without AllowArchivingWithJavaAgent)
thanks,
Calvin
On 11/16/18, 11:16 AM, serguei.spit...@oracle.com wrote:
On 11/16/18 10:41, serguei.spit...@oracle.com wrote:
Hi Ioi,
Thank you for the clarifications!
But then I have one more question to the fix in webrev.
http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/runtime/thread.cpp.udiff.html
* // Create agents for -agentlib: -agentpath: and converted
-Xrun*
* // Invokes Agent_OnLoad*
* // Called very early -- before JavaThreads exist*
* void Threads::create_vm_init_agents() {*
*+ if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");*
*+ }*
* extern struct JavaVM_ main_vm;*
* AgentLibrary* agent;*
* *
* JvmtiExport::enter_onload_phase();*
* *
* for (agent = Arguments::agents(); agent != NULL; agent =
agent->next()) {*
*+ // CDS dumping does not support native JVMTI agent*
*+ if (DumpSharedSpaces&& !agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support
native JVMTI agent, name", agent->name());*
*+ }*
*+*
* OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);*
* *
* if (on_load_entry != NULL) {*
* // Invoke the Agent_OnLoad function*
* jint err = (*on_load_entry)(&main_vm, agent->options(),
NULL);*
If !*agent->is_instrument_lib()* is passed then it will be
rejected with the message:*
"Must enable AllowArchivingWithJavaAgent in order to run Java
agent during CDS dumping"*
which is incorrect.
Probably, the fix needs to be something like this:
* for (agent = Arguments::agents(); agent != NULL; agent =
agent->next()) {*
*+ // CDS dumping does not support native JVMTI agent*
*+ if (DumpSharedSpaces&& !agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support
native JVMTI agent, name", agent->name());*
*+ }*
*+ // CDS dumping does not support native JVMTI agent*
*+ else if (DumpSharedSpaces&& !agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support
native JVMTI agent, name", agent->name());*
*+ }*
Sorry, somehow I've copy-pasted wrong fragment.
It has to be:
* for (agent = Arguments::agents(); agent != NULL; agent =
agent->next()) {*
*+ // CDS dumping does not support native JVMTI agent*
*+ if (DumpSharedSpaces) {
+ if(!agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support
native JVMTI agent, name", agent->name());*
*+ }*
*+ else if (!AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to
run Java agent during CDS dumping");*
*+ }
+ }
*
And this fragment at the begin of the function needs to be removed:
*+ if (DumpSharedSpaces&& !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping");*
*+ }*
Thanks,
Serguei
Thanks,
Serguei
On 11/16/18 10:18, Ioi Lam wrote:
On 11/16/18 9:45 AM, serguei.spit...@oracle.com wrote:
Hi Calvin,
On 11/16/18 09:26, Calvin Cheung wrote:
Serguei, Dan,
Thanks for taking a look.
I think the option name is correct but the comment for
Threads::create_vm_init_agents() is incorrect as I believe it
will start both Java native agents. Otherwise, my new tests
won't work.
It works because there is the system JVMTI (native) agent
library libinstrument.so that is supporting java agents.
It means that new option first impacts native agents and
transitively java agents as well via the libinstrument.so.
Dan already asked the question about your original
reasoning/intention.
If your intention is to control java agent only then the fix is
wrong as the real impact is much bigger.
If you wanted to control all the agents then the new option name
is misleading.
The intention of this change is:
Disallow all use of native and java agents by default. -- using
agents during dump time will cause undesirable side effects and
make the CDS archive unsuitable for production environments.
However, for testing CDS itself, we want to use the Java agent
(to run arbitrary Java code during dump time, such as causing GC
at specific times).
As a result, what we are doing is:
+ Always disallow non-Java agents.
+ Allow java agents only if AllowArchivingWithJavaAgent is
specified.
I think we should clean up the bug title and description if
necessary. However, the behavior of this patch is what we want.
And I think the naming of the AllowArchivingWithJavaAgent option
is correct (as least in the sense of "anything not explicitly
allow are strictly forbidden" :-)
Thanks
- Ioi
Below is my understanding:
In thread.cpp:
3697 if (Arguments::init_agents_at_startup()) {
3698 create_vm_init_agents();
3699 }
init_agents_at_startup() checks if the _agentList is empty in
arguments.hpp:
static bool init_agents_at_startup() { return
!_agentList.is_empty(); }
In arguments.cpp, entries will be added to the _agentList
during the parsing of the -agentLib, -agentPath, and -javaagent
arguments. See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be
populated via the add_init_agent() function. For the
-javaagent, the _agentList will be populated via the
add_instrument_agent() function.
So in create_vm_init_agents(), it will just loop through the
_agentList and starts all the agents. There's a check for
!agent->is_instrument_agent() at line 4092 which is for not
allowing any JVMTI (native) agent to be run during dump time.
One more reply to Serguei's comment below...
Okay, thank you for the update!
Thanks,
Serguei
thanks,
Calvin
On 11/16/18, 6:08 AM, Daniel D. Daugherty wrote:
> The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
> and Agent_OnLoad is an entrypoint in native agent library.
The '-agentlib' and '-agentpath' options are used to load and
execute
a native library via the Agent_OnLoad() entry point. However, the
agent does not have to use JVM/TI (it could be pure JNI):
$ java -help
-agentlib:<libname>[=<options>]
load native agent library <libname>, e.g.
-agentlib:hprof
see also, -agentlib:jdwp=help and
-agentlib:hprof=help
-agentpath:<pathname>[=<options>]
load native agent library by full pathname
The '-javaagent' option is used to load and execute a Java agent:
-javaagent:<jarpath>[=<options>]
load Java programming language agent, see
java.lang.instrument
so I agree that the option is misnamed (a bit), but the reasons
needed to be clarified.
Dan
On 11/16/18 1:39 AM, serguei.spit...@oracle.com wrote:
Hi Calvin,
It seems the option name AllowArchivingWithJavaAgent is not
right.
In fact, it is about JVMTI (native) agents:
*// Create agents for -agentlib: -agentpath: and converted
-Xrun*
*// Invokes Agent_OnLoad*
*// Called very early -- before JavaThreads exist*
*void Threads::create_vm_init_agents() {*
*+ if (DumpSharedSpaces && !AllowArchivingWithJavaAgent) {*
*+ vm_exit_during_cds_dumping(*
*+ "Must enable AllowArchivingWithJavaAgent in order to run
Java agent during CDS dumping");*
*+ }*
*extern struct JavaVM_ main_vm;*
*AgentLibrary* agent;*
**
*JvmtiExport::enter_onload_phase();*
**
*for (agent = Arguments::agents(); agent != NULL; agent =
agent->next()) {*
*+ // CDS dumping does not support native JVMTI agent*
*+ if (DumpSharedSpaces && !agent->is_instrument_lib()) {*
*+ vm_exit_during_cds_dumping("CDS dumping does not support
native JVMTI agent, name", agent->name());*
*+ }*
*+ *
*OnLoadEntry_t on_load_entry = lookup_agent_on_load(agent);*
The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
and Agent_OnLoad is an entrypoint in native agent library.
So, I'm thinking if it'd make sense to rename the option to
something like below:
AllowArchivingWithJVMTIAgent or
AllowArchivingWithNativeAgent or
AllowArchivingWithAgent
http://cr.openjdk.java.net/%7Eccheung/8201375/webrev.00/src/hotspot/share/memory/filemap.cpp.frames.html
212 _allow_archiving_with_java_agent =
AllowArchivingWithJavaAgent; ...
1350 if (_allow_archiving_with_java_agent &&
!AllowArchivingWithJavaAgent) { 1351
FileMapInfo::fail_continue("The setting of the
AllowArchivingWithJavaAgent is different " 1352 "from the
setting in the shared archive."); 1353 return false; 1354 }
The _allow_archiving_with_java_agent is initialized with the
AllowArchivingWithJavaAgent at line 212.
The assignment at line 212 is for populating the archive header
during CDS dump time.
It is not clear from scratch how these two values can be
different at line 1350. At least, some comment explaining it
is needed.
The value retrieved from the archive header could be different
from the run time setting.
I will add a comment.
Thanks,
Serguei
On 11/15/18 10:56, Jiangli Zhou wrote:
+1
Adding serviceability-dev mailing list. It would be good to
have this reviewed by the JVMTI experts also to make sure
all cases are covered.
Thanks,
Jiangli
On 11/15/18 9:29 AM, Ioi Lam wrote:
Hi Calvin,
The changes look good.
Thanks
- Ioi
On 11/14/18 4:40 PM, Calvin Cheung wrote:
bug: https://bugs.openjdk.java.net/browse/JDK-8201375
webrev:
http://cr.openjdk.java.net/~ccheung/8201375/webrev.00/
This change is for adding a diagnostic vm option
(AllowArchivingWithJavaAgent) to allow the -javaagent in
the command line during CDS dumping. Please refer to the
corresponding CSR
(https://bugs.openjdk.java.net/browse/JDK-8213813) for
details.
Testing: ran hs-tier{1,2,3} tests through mach5.
thanks,
Calvin
