[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR - Changes to address CCC 8014135: Support for statically linked agents
From: bill.pittore () oracle ! com
Date: 2013-07-30 19:17:31
Message-ID: 51F8114B.2040302 () oracle ! com
[Download RAW message or body]
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@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@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
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Thanks Serguei for the comments. Some comments inline. I updated the
webrevs at<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/">http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/</a><br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/</a><br>
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bpittore/8014135/javadoc/index.html">http://cr.openjdk.java.net/~bpittore/8014135/javadoc/index.html</a><br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/jvmti.html">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/jvmti.html</a><br>
<br>
<br>
On 7/26/2013 5:00 AM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote: \
<blockquote type="cite"> <meta content="text/html; charset=UTF-8" \
http-equiv="Content-Type"> <div class="moz-cite-prefix">Hi Bill,<br>
<br>
Sorry for the big delay.<br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/jdk/webrev.01/">http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.01/</a><br>
<br>
<br>
src/share/classes/com/sun/tools/attach/VirtualMachine.java:<br>
<br>
<br>
I'm suggesting to use the reference \
<b><code>Agent_OnAttach[_L]</code></b><b><code></code></b> even more \
consistently.<br> (If, in some cases, you prefer the longer form to underline the
difference between<br>
dynamically and statically linked libraries then feel free to
leave it as it is.)<br>
<br>
It would simplify the following fragments:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre>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</pre> \
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre> ==>
304 * It then causes the target VM to invoke the \
<code>Agent_OnAttach[_L]</code> function</pre> <br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre>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</pre> ==><br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> 409 * It then causes the target VM to invoke the \
<code>Agent_OnAttach[_L]</code> 410 * function as specified in \
the</pre> <br>
</div>
</blockquote>
I left the above as is since it's part of the method description.
The following fragments below I simplified as you suggested.<br>
<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
the following 4 identical fragments:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> 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.</pre>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> 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.</pre>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> 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 </pre> <meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre> 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.</pre>
==><br>
<br>
336 * If the
<code>Agent_OnAttach[_L]</code> function returns an
error.<br>
<br>
</div>
</blockquote>
<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/</a><br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/jvmti.html">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html</a><br>
src/share/vm/prims/jvmti.xml<br>
<br>
Lines 442-462: many extra <p/>'s. The fragment does not
look well.<br>
I'd suggest to remove most of them.<br>
Also, these lines are too long. Could you make them shorter,
please?<br>
The same is applied to other long new lines in this file.<br>
</div>
</blockquote>
Cleaned this up a bit.<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
Lines 490-491, 502-503, 505-506:<br>
The same sentence is repeated 3 times: "the agent library
may be statically linked ..."<br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<span class="new"> <br>
490 Note that the agent library may be statically linked
into the executable</span><br>
<span class="new">491 in which case no actual loading takes
place.</span><br>
</div>
</blockquote>
Fixed.<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
501
<code>-agentpath:c:\myLibs\foo.dll=opt1,opt2</code>
is specified, the VM will attempt to <br>
502 load the shared library
<code>c:\myLibs\foo.dll</code>. As mentioned above,
the agent library may be statically linked into the executable<br>
503 in which case no actual loading takes place<br>
<br>
505 Note that the agent library may be statically linked
into the executable<br>
506 in which case no actual loading takes place.<br>
<br>
</div>
</blockquote>
Tweaked the above a bit to make it less wordy.<br>
<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> Lines 677-678: The dot is missed at
the end of line 677:<br>
<br>
677 and enabled the event<br>
<br>
</div>
</blockquote>
Fixed.<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
src/os/posix/vm/os_posix.cpp<br>
<br>
- no comments<br>
<br>
src/os/windows/vm/os_windows.cpp<br>
<br>
- no comments<br>
<br>
src/share/vm/prims/jvmtiExport.cpp<br>
<br>
- no comments<br>
<br>
src/share/vm/runtime/arguments.hpp<br>
<br>
- no comments<br>
<br>
src/share/vm/runtime/os.cpp<br>
<br>
Space is missed after the 'if':<br>
471 if(entryName != NULL) {<br>
<br>
</div>
</blockquote>
Fixed.<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> Extra space after the '*':<br>
483 void * saveHandle;<br>
<br>
</div>
</blockquote>
Fixed.<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> src/share/vm/runtime/os.hpp<br>
<br>
- no comments<br>
<br>
src/share/vm/runtime/thread.cpp<br>
<br>
The line has been removed:<br>
3866 break;<br>
<br>
</div>
</blockquote>
Correct, the inner for loop was removed so no need for the break;<br>
<blockquote type="cite">
<div class="moz-cite-prefix"> <br>
I'm still in process of reading the code.<br>
Another pass is needed to make sure that nothing is missed.<br>
But in general, the code quality is pretty good.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 7/25/13 10:47 AM, <a class="moz-txt-link-abbreviated"
href="mailto:bill.pittore@oracle.com">bill.pittore@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:51F164CF.50307@oracle.com" type="cite">Still
need an official reviewer for the hotspot changes for statically
linked agents. <br>
<br>
thanks, <br>
bill <br>
<br>
<blockquote type="cite">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). <br>
The JEP for this change is the same JEP as the JNI changes: <br>
<a class="moz-txt-link-freetext"
href="http://openjdk.java.net/jeps/178">http://openjdk.java.net/jeps/178</a>
<br>
<br>
Webrevs are here: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/jdk/webrev.00/">http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.00/</a>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/</a>
<br>
<br>
The changes to jvmti.xml can also be seen in the output file
that I placed here: <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ebpittore/8014135/hotspot/webrev.00/jvmti.html">http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.00/jvmti.html</a>
<br>
<br>
Thanks, <br>
bill <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic