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 19:01:59 UTC 2018
On 11/16/18 10:52 AM, Daniel D. Daugherty wrote:
> On 11/16/18 1:18 PM, 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" :-)
>
> I'm not sure that a Java agent can exist without the presence of the
> backing native agent. For JLI/JPLIS, you have both the Java agent part
> and the native agent that supports the Java agent.
>
> So if you're truly disallowing non-Java agents, I'm not sure how that
> Java agent can run without the backing native agent. However, this area
> may have evolved since I last worked on it...
>
> Dan
>
Hi Dan,
You're correct that Java agents are implemented on top of native agents.
However, Java agents expose a more restricted set of capabilities than
the native agents. Since our CDS test only needs to use the
instrumentation provided by Java agents, it's safer to allow only Java
agents.
I should have said "forbid the direct use of native agents via the
-agentlib and -agentpath command-line options".
Thanks
- Ioi
>
>>
>> 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