RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping
Ioi Lam
ioi.lam at oracle.com
Fri Nov 16 18:18:29 UTC 2018
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