On 11/16/18 10:52 AM, Daniel D. Daugherty wrote:
On 11/16/18 1:18 PM, Ioi Lam wrote:
On 11/16/18 9:45 AM, 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" :-)
I'm not sure that a Java agent can exist without the presence of the
backing native agent. For JLI/JPLIS, you have both the Java agent part
and the native agent that supports the Java agent.
So if you're truly disallowing non-Java agents, I'm not sure how that
Java agent can run without the backing native agent. However, this area
may have evolved since I last worked on it...
Dan
Hi Dan,
You're correct that Java agents are implemented on top of native agents.
However, Java agents expose a more restricted set of capabilities than
the native agents. Since our CDS test only needs to use the
instrumentation provided by Java agents, it's safer to allow only Java
agents.
I should have said "forbid the direct use of native agents via the
-agentlib and -agentpath command-line options".
Thanks
- Ioi
Thanks
- Ioi
Below is my understanding:
In thread.cpp:
3697 if (Arguments::init_agents_at_startup()) {
3698 create_vm_init_agents();
3699 }
init_agents_at_startup() checks if the _agentList is empty in
arguments.hpp:
static bool init_agents_at_startup() { return
!_agentList.is_empty(); }
In arguments.cpp, entries will be added to the _agentList during
the parsing of the -agentLib, -agentPath, and -javaagent arguments.
See lines around 2463-2502.
For the -agentLib and -agentPath args, the _agentList will be
populated via the add_init_agent() function. For the -javaagent,
the _agentList will be populated via the add_instrument_agent()
function.
So in create_vm_init_agents(), it will just loop through the
_agentList and starts all the agents. There's a check for
!agent->is_instrument_agent() at line 4092 which is for not
allowing any JVMTI (native) agent to be run during dump time.
One more reply to Serguei's comment below...
Okay, thank you for the update!
Thanks,
Serguei
thanks,
Calvin
On 11/16/18, 6:08 AM, Daniel D. Daugherty wrote:
> The flags -agentlib and -agentpath are to run JVMTI (native)
agents,
> and Agent_OnLoad is an entrypoint in native agent library.
The '-agentlib' and '-agentpath' options are used to load and execute
a native library via the Agent_OnLoad() entry point. However, the
agent does not have to use JVM/TI (it could be pure JNI):
$ java -help
-agentlib:<libname>[=<options>]
load native agent library <libname>, e.g.
-agentlib:hprof
see also, -agentlib:jdwp=help and
-agentlib:hprof=help
-agentpath:<pathname>[=<options>]
load native agent library by full pathname
The '-javaagent' option is used to load and execute a Java agent:
-javaagent:<jarpath>[=<options>]
load Java programming language agent, see
java.lang.instrument
so I agree that the option is misnamed (a bit), but the reasons
needed to be clarified.
Dan
On 11/16/18 1:39 AM, 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
