RFR - Changes to address CCC 8014135: Support for statically linked agents
BILL PITTORE
bill.pittore at oracle.com
Mon Aug 19 12:11:15 PDT 2013
On 8/6/2013 5:47 AM, serguei.spitsyn at oracle.com wrote:
> 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.
>
Fixed up the comment and variable names to make more sense.
>
> 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.
>
Fixed.
>
> src/share/vm/runtime/thread.cpp
>
> Minor: It is better to initialize the below with NULL:
> 3699 void *library;
>
Fixed.
>
> 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!
>
You're welcome!
thanks for the review, I'm rebuilding/testing and then will send out a
new webrev.
bill
> Thanks,
> Serguei
>
> On 8/5/13 11:59 AM, serguei.spitsyn at 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.spitsyn at oracle.com wrote:
>>>
>>>> On 8/2/13 8:11 PM, Bill Pittore wrote:
>>>>> On 8/2/2013 9:12 PM, serguei.spitsyn at 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.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