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 21:04:17 UTC 2018
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.
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/8829be6a/attachment-0001.html>
More information about the serviceability-dev
mailing list