RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Nov 16 23:22:01 UTC 2018
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