RFR - Changes to address CCC 8014135: Support for statically linked agents
BILL PITTORE
bill.pittore at oracle.com
Tue Aug 20 18:18:15 PDT 2013
Thanks Serguei for the detailed review. I think I captured all the
tweaks you mentioned and have new webrevs at:
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.03/
http://cr.openjdk.java.net/~bpittore/8014135/jdk/webrev.04/
also you can find the jvmti.html output file at
http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.03/jvmti.html
and the javadoc from VirtualMachine.java at:
http://cr.openjdk.java.net/~bpittore/8014135/javadoc/
bill
On 8/20/2013 6:42 PM, serguei.spitsyn at oracle.com wrote:
> 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