RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping
Calvin Cheung
calvin.cheung at oracle.com
Fri Nov 16 23:34:03 UTC 2018
Thanks, Serguei!
Calvin
On 11/16/18, 3:22 PM, serguei.spitsyn at oracle.com wrote:
> Hi Calvin,
>
> It looks good to me.
> No need in new webrev if you update this comment as below.
>
> Thanks,
> Serguei
>
>
> On 11/16/18 2:00 PM, Calvin Cheung wrote:
>>
>>
>> On 11/16/18, 1:04 PM, serguei.spitsyn at 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.spitsyn at oracle.com wrote:
>>>>> On 11/16/18 10:41, serguei.spitsyn at 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.spitsyn at 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.spitsyn at 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
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>
More information about the serviceability-dev
mailing list