RFR - Changes to address CCC 8014135: Support for statically linked agents
bill.pittore at oracle.com
bill.pittore at oracle.com
Tue Aug 20 09:55:00 PDT 2013
I've updated the hotspot and jdk webrevs based on Coleen's and Serguei's
comments.
http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.03/
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.02/
thanks,
bill
On 8/19/2013 1:13 PM, BILL PITTORE wrote:
> On 8/5/2013 6:13 PM, Coleen Phillimore wrote:
>>
>> Hi Bill, I have some high level comments and other comments. This
>> wasn't as easy to review as Bob promised me!
> :-P
>>
>> 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.
>> *
> Fixed.
>> 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.
> Fixed all names. Original code came from JDK which is why I had this
> scheme.
>>
>> 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?
> Added a comment to the above function to describe what's going on. For
> absolute path case, the lib_name is something like /a/b/libL.so so we
> need to strip off the path, the 'lib' prefix and the '.so' suffix to
> get the base name.
>> 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.
>>
> Ok, changed it to the above.
>> 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?
> Changed it to '@XX'. It's really the length of the arguments which
> could change in the future, so XX should be fine.
>>
>> 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!
>>
> Fixed.
>> 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?
>>
> Fixed.
>>
>> 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.
> Re-worded this to be more explicit about statically linked in
>>
>> Maybe find_statically_linked_agent() would be a better name for this
>> function?
> I think builtin came from when we did original JNI builitin libraries.
> It might be documented as such in the JEP or CCC.
>>
>> Is the is_valid() function really mean has_been_loaded()?
>> 2236 if (agentLib->valid()) {
> Yes, either it was found statically linked into the executable or it
> was successfully loaded as a shared lib.
>>
>>
>> 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.
> Ran the vm/nsk tests using UTE. Also we have some new test code to
> test the statically linked agent functionality.
>
> Thanks so much for the excellent review.
> I'll update the webrev as soon as I rebuild and test.
>
> bill
>
>>
>> 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