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 20:09:28 UTC 2018
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/87770165/attachment-0001.html>
More information about the serviceability-dev
mailing list