RFR - Changes to address CCC 8014135: Support for statically linked agents
bill.pittore at oracle.com
bill.pittore at oracle.com
Tue Jul 30 12:17:31 PDT 2013
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/20130730/60feafa8/attachment.html
More information about the serviceability-dev
mailing list