RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Aug 6 02:47:47 PDT 2013
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.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 serviceability-dev
mailing list