RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Aug 6 12:10:31 PDT 2013
On 8/6/13 11:57 AM, serguei.spitsyn at oracle.com wrote:
> On 8/5/13 7:41 AM, Bob Vandette wrote:
>> On Aug 2, 2013, at 11:11 PM, Bill Pittore wrote:
>>
>>> On 8/2/2013 9:12 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Bill,
>>>>
>>>> A couple of more questions.
>>>>
>>>> webrev.01/jvmti.html
>>>>
>>>> An agent L whose image has been combined with the VM *is defined*
>>>> as /statically linked/
>>>> if and only if the agent exports a function called Agent_OnLoad_L.
>>>>
>>>> A question to the above.
>>>> Are we going to allow to link a library dynamically if it exports both
>>>> the Agent_OnLoad and Agent_OnLoad_L functions?
>>>> It can be convenient if a library exports both Agent_OnLoad and
>>>> Agent_OnLoad_L
>>>> as it can be linked statically or dynamically depending on the need
>>>> without changes.
>>>>
>>> It would be nice but the problem is that you could only link one
>>> agent into the VM if it exported Agent_OnLoad. Otherwise there would
>>> be a symbol collision with the second agent you linked in that also
>>> had Agent_OnLoad. As an agent developer you will have to select one
>>> or the other at build time, either statically linked in or dynamic.
>>>> You already added the following statement to the JVMTI spec:
>>>> If a /statically linked/ agent L exports a function called
>>>> Agent_OnLoad_L and
>>>> a function called Agent_OnLoad, the Agent_OnLoad function will be
>>>> ignored.
>>>>
>>>> Could we say it in a shorter form?:
>>>> If a /statically linked/ agent L exports a function called
>>>> Agent_OnLoad,
>>>> the Agent_OnLoad function will be ignored.
>>> I believe I copied this from JNI static linking JEP. If so, I'll
>>> probably leave it as is just for consistency with JNI static spec.
>>> JVM TI static linking spec is closely related to JNI static linking
>>> spec.
>>>> In this context would it be reasonable to add another statement:
>>>> If a /dynamically linked/ agent L exports a function called
>>>> Agent_OnLoad_L,
>>>> the Agent_OnLoad_L function will be ignored.
>>>>
>> I'd rather leave this undefined since the behavior is very platform
>> specific.
>> The way we determine if a library is statically linked is by the
>> presence of the Agent_OnLoad_L function.
>> If this function exists in a dynamically linked library, it will be
>> treated as a static library if by some chance
>> it's attempted to be loaded twice, since the symbol will may be
>> visible in the running process.
> (I've added Alan and Coleen to the cc-list)
>
> It is still not clear to me how the second load might happen.
It is inaccurate statement above.
I wanted to say "such a second load" that will determine the dynamically
loaded agent as statically loaded.
Thanks,
Serguei
>
> The agent is determined as statically loaded if the Agent_OnLoad_L
> function
> is found in the main program, not in the dynamic library.
> At least, the dll_lookup is done for the main program (for
> DefaultProcessHandle).
> If the agent is loaded dynamically, the Agent_OnLoad_L function in the
> agent library
> can be ignored because the dll_lookup in the main program will not
> find it.
> In a case of the second load this function will be ignored again.
> So that there is no conflict with the dynamically loaded agent here.
>
> We can skip this issue for now.
> Alan is going to review the JEP from the dynamic Attach point of view
> and hopefully will pay attention to these details.
>
> Thanks,
> Serguei
>
>>
>> Bob.
>>
>>
>>>> The same questions apply to the Agent_OnAttach and Agent_OnAttach_L
>>>> entry points.
>>>>
>>> I'm out on vacation for a couple of weeks so I'll leave it up to Bob
>>> V. and yourself if you guys want to hash out better/different wording.
>>>
>>> bill
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 7/30/13 12:17 PM, bill.pittore at oracle.com wrote:
>>>>> Thanks Serguei for the comments. Some comments inline. I updated
>>>>> the webrevs at
>>>>> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/
>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/
>>>>>
>>>>> http://cr.openjdk.java.net/~bpittore/8014135/javadoc/index.html
>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/jvmti.html
>>>>>
>>>>>
>>>>>
>>>>> On 7/26/2013 5:00 AM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Bill,
>>>>>>
>>>>>> Sorry for the big delay.
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.01/
>>>>>>
>>>>>>
>>>>>> src/share/classes/com/sun/tools/attach/VirtualMachine.java:
>>>>>>
>>>>>>
>>>>>> I'm suggesting to use the reference
>>>>>> *<code>Agent_OnAttach[_L]</code>**||* even more consistently.
>>>>>> (If, in some cases, you prefer the longer form to underline the
>>>>>> difference between
>>>>>> dynamically and statically linked libraries then feel free to
>>>>>> leave it as it is.)
>>>>>>
>>>>>> It would simplify the following fragments:
>>>>>>
>>>>>> 304 * It then causes the target VM to invoke the
>>>>>> <code>Agent_OnAttach</code> function
>>>>>> 305 * or, for a statically linked agent named 'L', the
>>>>>> <code>Agent_OnAttach_L</code> function
>>>>>> ==>
>>>>>>
>>>>>> 304 * It then causes the target VM to invoke the
>>>>>> <code>Agent_OnAttach[_L]</code> function
>>>>>>
>>>>>> 409 * It then causes the target VM to invoke the
>>>>>> <code>Agent_OnAttach</code>
>>>>>> 410 * function or, for a statically linked agent named 'L', the
>>>>>> 411 * <code>Agent_OnAttach_L</code> function as specified in
>>>>>> the
>>>>>> ==>
>>>>>> 409 * It then causes the target VM to invoke the
>>>>>> <code>Agent_OnAttach[_L]</code>
>>>>>> 410 * function as specified in the
>>>>>>
>>>>> I left the above as is since it's part of the method description.
>>>>> The following fragments below I simplified as you suggested.
>>>>>
>>>>>> the following 4 identical fragments:
>>>>>>
>>>>>> 341 * If the <code>Agent_OnAttach</code> function
>>>>>> returns an error
>>>>>> 342 * or, for a statically linked agent named
>>>>>> 'L', if the
>>>>>> 343 * <code>Agent_OnAttach_L</code> function returns
>>>>>> 344 * an error.
>>>>>> 375 * If the <code>Agent_OnAttach</code> function
>>>>>> returns an error
>>>>>> 376 * or, for a statically linked agent named
>>>>>> 'L', if the
>>>>>> 377 * <code>Agent_OnAttach_L</code> function returns
>>>>>> 378 * an error.
>>>>>> 442 * If the <code>Agent_OnAttach</code> function
>>>>>> returns an error
>>>>>> 443 * or, for a statically linked agent named
>>>>>> 'L', if the
>>>>>> 444 * <code>Agent_OnAttach_L</code> function returns an error
>>>>>> 475 * If the <code>Agent_OnAttach</code> function
>>>>>> returns an error
>>>>>> 476 * or, for a statically linked agent named
>>>>>> 'L', if the
>>>>>> 477 * <code>Agent_OnAttach_L</code> function returns
>>>>>> 478 * an error.
>>>>>> ==>
>>>>>>
>>>>>> 336 * If the <code>Agent_OnAttach[_L]</code>
>>>>>> function returns an error.
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/
>>>>>>
>>>>>>
>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html
>>>>>>
>>>>>> src/share/vm/prims/jvmti.xml
>>>>>>
>>>>>> Lines 442-462: many extra <p/>'s. The fragment does not look well.
>>>>>> I'd suggest to remove most of them.
>>>>>> Also, these lines are too long. Could you make them shorter, please?
>>>>>> The same is applied to other long new lines in this file.
>>>>> Cleaned this up a bit.
>>>>>> Lines 490-491, 502-503, 505-506:
>>>>>> The same sentence is repeated 3 times: "the agent library
>>>>>> may be statically linked ..."
>>>>>>
>>>>>> 490 Note that the agent library may be statically linked into
>>>>>> the executable
>>>>>> 491 in which case no actual loading takes place.
>>>>> Fixed.
>>>>>> 501 <code>-agentpath:c:\myLibs\foo.dll=opt1,opt2</code> is
>>>>>> specified, the VM will attempt to
>>>>>> 502 load the shared library
>>>>>> <code>c:\myLibs\foo.dll</code>. As mentioned above, the agent
>>>>>> library may be statically linked into the executable
>>>>>> 503 in which case no actual loading takes place
>>>>>>
>>>>>> 505 Note that the agent library may be statically linked into
>>>>>> the executable
>>>>>> 506 in which case no actual loading takes place.
>>>>>>
>>>>> Tweaked the above a bit to make it less wordy.
>>>>>
>>>>>> Lines 677-678: The dot is missed at the end of line 677:
>>>>>>
>>>>>> 677 and enabled the event
>>>>>>
>>>>> Fixed.
>>>>>> src/os/posix/vm/os_posix.cpp
>>>>>>
>>>>>> - no comments
>>>>>>
>>>>>> src/os/windows/vm/os_windows.cpp
>>>>>>
>>>>>> - no comments
>>>>>>
>>>>>> src/share/vm/prims/jvmtiExport.cpp
>>>>>>
>>>>>> - no comments
>>>>>>
>>>>>> src/share/vm/runtime/arguments.hpp
>>>>>>
>>>>>> - no comments
>>>>>>
>>>>>> src/share/vm/runtime/os.cpp
>>>>>>
>>>>>> Space is missed after the 'if':
>>>>>> 471 if(entryName != NULL) {
>>>>>>
>>>>> Fixed.
>>>>>> Extra space after the '*':
>>>>>> 483 void * saveHandle;
>>>>>>
>>>>> Fixed.
>>>>>> src/share/vm/runtime/os.hpp
>>>>>>
>>>>>> - no comments
>>>>>>
>>>>>> src/share/vm/runtime/thread.cpp
>>>>>>
>>>>>> The line has been removed:
>>>>>> 3866 break;
>>>>>>
>>>>> Correct, the inner for loop was removed so no need for the break;
>>>>>> I'm still in process of reading the code.
>>>>>> Another pass is needed to make sure that nothing is missed.
>>>>>> But in general, the code quality is pretty good.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 7/25/13 10:47 AM, bill.pittore at oracle.com wrote:
>>>>>>> Still need an official reviewer for the hotspot changes for
>>>>>>> statically linked agents.
>>>>>>>
>>>>>>> thanks,
>>>>>>> bill
>>>>>>>
>>>>>>>> These changes address bug 8014135 which adds support for
>>>>>>>> statically linked agents in the VM. This is a followup to the
>>>>>>>> recent JNI spec changes that addressed statically linked JNI
>>>>>>>> libraries( 8005716).
>>>>>>>> The JEP for this change is the same JEP as the JNI changes:
>>>>>>>> http://openjdk.java.net/jeps/178
>>>>>>>>
>>>>>>>> Webrevs are here:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/
>>>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/
>>>>>>>>
>>>>>>>> The changes to jvmti.xml can also be seen in the output file
>>>>>>>> that I placed here:
>>>>>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> bill
>>>>>>>>
>>>>>
>
More information about the serviceability-dev
mailing list