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.spit...@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.pitt...@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
