RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Aug 2 21:34:20 PDT 2013
On 8/2/13 8: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.
I did not want to use the Agent_OnLoad for statically linked agent.
Just wanted to say that the presence of the Agent_OnLoad_L must be ignored
if the agent is linked dynamically.
Maybe this rule needs to be clearly stated in the JVMTI spec.
>> 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.
I see. Then it is Ok with me.
>>
>> 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.
>>
>> 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.
Thank you for the quick reply, and have a nice vacation!
Thanks,
Serguei
>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130802/e4355e0e/attachment.html
More information about the serviceability-dev
mailing list