RFR(M): 8201375: Add the AllowArchivingWithJavaAgent diagnostic vm option to allow the use of the -javaagent option during CDS dumping

[Calvin Cheung]

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.

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...

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
>>>
>>
>