RFR - Changes to address CCC 8014135: Support for statically linked agents
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 5 15:13:06 PDT 2013
Hi Bill, I have some high level comments and other comments. This
wasn't as easy to review as Bob promised me!
http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.02/src/share/classes/com/sun/tools/attach/VirtualMachine.java.udiff.html
paramter (typo - two instances)
* @throws AgentInitializationException
- * If the <code>Agent_OnAttach</code> function returns an error
+ * If the <code>Agent_OnAttach[_L]</code> function returns an error.
+ * an error.
*
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/os/posix/vm/os_posix.cpp.frames.html
nameLen, prefixLen, suffixLen - the JVM coding convention (unlike Java)
is to have variable names lower case and separated by a _ (not
camelcase). Same for all the new code here buildAgentFunctionName.
Can you put a function above this function to say what it does? So once
it's code reviewed, we can quickly know what it does without having to
study this again - ie. Is name something like libxxx.so and you're
trying to extract lib and .so from it? There's no verification of this
at lines 286 and 287 but the code is handling the case of other sorts of
unexpected input (ie line 283). Why don't you strip the lib and the
.so if !is_absolute_path?
291 agentEntryName = NEW_C_HEAP_ARRAY(char, len, mtThread);
292 if (agentEntryName == NULL) {
293 return NULL;
294 }
If NEW_C_HEAP_ARRAY fails it terminates the JVM. I think you might
want this function instead NEW_C_HEAP_ARRAY_RETURN_NULL if there's a
reasonable recovery possible.
os_windows.cpp
Same comments as above. Also:
5432 strncat(agentEntryName, name, nameLen);
5433 strcat(agentEntryName, p);
5434 //agentEntryName == _Agent_OnLoad_libName at 12
5435 } else {
You could say why 12 but shouldn't it be the length of the resulting
symbol instead and not 12?
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/runtime/os.cpp.frames.html
findAgentFunction - same comment about the coding conventions. functions
and variables are lower case with underscores. Class names are
CamelCase. It would be convenient if the jvm code used the java coding
convention but the rest of it doesn't so this new code is
inconsistent. This is hard to follow as it is!
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/runtime/thread.cpp.frames.html
3826 size_t num_symbol_entries = sizeof(on_unload_symbols) / sizeof(char*);
Why not use ARRAY_SIZE macro?
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.01/src/share/vm/prims/jvmtiExport.cpp.frames.html
2208 // Check for builtin agent. If not then if the path is absolute we attempt
2209 // to load the library. Otherwise we try to load it from the standard
2210 // dll directory.
I think you are missing the word "found" in this sentence. You are
using builtin agent to mean agent defined in a statically linked
library, I believe. Can you say that instead? Builtin means that the
jvm knows about it and implements it but in this case it doesn't.
Maybe find_statically_linked_agent() would be a better name for this
function?
Is the is_valid() function really mean has_been_loaded()?
2236 if (agentLib->valid()) {
Did you run the Internal NSK vm.quick.testlist tests on this to verify
that nothing breaks? There are a lot of tests with different agents and
combinations in that suite and it frequently holds some surprises in
this area so best to run it first. Let me know if you need help.
Coleen
On 08/05/2013 10:41 AM, Bob Vandette wrote:
> On Aug 2, 2013, at 11: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.
>>> 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.
>>> 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.
>>>
> I'd rather leave this undefined since the behavior is very platform specific.
> The way we determine if a library is statically linked is by the presence of the Agent_OnLoad_L function.
> If this function exists in a dynamically linked library, it will be treated as a static library if by some chance
> it's attempted to be loaded twice, since the symbol will may be visible in the running process.
>
> Bob.
>
>
>>> 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.
>>
>> 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