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 22:00:46 UTC 2018
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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181116/e38dd170/attachment-0001.html>
More information about the serviceability-dev
mailing list