RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Nov 16 19:03:53 UTC 2018
On 11/16/18 2:01 PM, Ioi Lam wrote:
>
>
> 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".
I'm good with that! Thanks for bearing with me...
Dan
>
> 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