Re: RFR - Changes to address CCC 8014135: Support for statically linked agents

[serguei.spit...@oracle.com]

Bob,

I've done another look and got a few more  comments below.
  http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/

  src/share/vm/runtime/os.cpp

  The comment before the function findAgentFunction() is inconsistent 
with the implementation.
  There is a mismatch in "lib name":  lib_name and libName .vs. name
  There is a mismatch in "check lib": check_lib .vs. checkLib
  The following part is not accurate as it does not tell anything about 
the condition is_static_lib():

+ * If check_lib == true then we are looking for an
+ * Agent_OnLoad_libname or Agent_OnAttach_libname function to determine if
+ * this library is statically linked into the image.


  src/os/posix/vm/os_posix.cpp
  src/os/windows/vm/os_windows.cpp

   The function buildAgentFunctionName():
   Minor: I'd suggest to change the argument name from "name" to 
"libname" or "lib_name".
              Otherwise, it takes time to figure out what the "name" 
argument really means.


  src/share/vm/runtime/thread.cpp

  Minor: It is better to initialize the below with NULL:
  3699   void *library;


  I also agree with Coleen on the following:
  - about using the hotspot coding convention for variables/functions names
  - a comment is needed before the function buildAgentFunctionName
  - built-in agent => statically linked agent


One more thing to say is that I really like the implementation.
Thank you for adding this feature in such a non-intrusive fashion!

Thanks,
Serguei

On 8/5/13 11:59 AM, serguei.spit...@oracle.com wrote:
Bob,

I'm not waiting for any changes from Bill at the moment but still 
reading the code.
Sorry for the latency but it takes time as not everything is clear to 
me yet.

Thanks,
Serguei

On 8/5/13 10:59 AM, Bob Vandette wrote:
Serguei,

Are you ok with the webrev at this point or are you waiting for any 
changes from Bill?

I've asked Coleen to review the code since she's an official Reviewer 
but she'd like to make
sure the serviceability team is ok with the changes.

Bob.


On Aug 3, 2013, at 12:34 AM, serguei.spit...@oracle.com wrote:

On 8/2/13 8:11 PM, Bill Pittore wrote:
On 8/2/2013 9:12 PM, serguei.spit...@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.pitt...@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.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