RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Aug 2 18:12:31 PDT 2013
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.
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.
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.
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 hotspot-dev
mailing list