RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Aug 20 15:42:57 PDT 2013
Hi Bill,
It looks good in general.
Some minor comments.
src/share/classes/com/sun/tools/attach/VirtualMachine.java
The copyright comment is out-of-date
|| src/os/posix/vm/os_posix.cpp
symName => sym_name
src/os/windows/vm/os_windows.cpp
5454 //agent_entry_name == _Agent_OnLoad_libName at XX
Need a space after //
||src/share/vm/prims/jvmti.xml
The copyright comment is out-of-date.
The lines are too long: 520-523, 584, 593, 623, 654, 660, 679, 689.
src/share/vm/prims/jvmtiExport.cpp
src/share/vm/runtime/arguments.hpp
No comments
||src/share/vm/runtime/os.cpp
The indentation is incorrect:
456 const char *syms[], size_t syms_len) {
The comments are cross-inconsistent (lib_name vs libname):
447 * Support for finding Agent_On(Un)Load/Attach<_lib_name> if it exists.
. . .
449 * Agent_OnLoad_lib_name or Agent_OnAttach_lib_name function to determine if
. . .
491 // Check for Agent_OnLoad/Attach_libname function
. . .
498 // Found an entry point like Agent_OnLoad_libname so we have a static agent
||src/share/vm/runtime/os.hpp
542 // return the handle of this process
543 static void* get_default_process_handle();
544
545 // Check for static linked agent library
546 static bool find_builtin_agent(AgentLibrary *agent_lib, const char *syms[],
547 size_t syms_len);
548
549 // Find agent entry point
550 static void *find_agent_function(AgentLibrary *agent_lib, bool check_lib,
551 const char *syms[], size_t syms_len);
The comment at the line #542 should start from capital letter.
Wrong indentation at the lines: #547, #551.
||src/share/vm/runtime/thread.cpp
Wrong indentation:
3748 on_load_entry = CAST_TO_FN_PTR(OnLoadEntry_t, os::find_agent_function(agent,
3749 false,
3750 on_load_symbols,
3751 num_symbol_entries));
Thanks,
Serguei
On 8/20/13 9:55 AM, bill.pittore at oracle.com wrote:
> 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