RFR - Changes to address CCC 8014135: Support for statically linked agents
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Aug 21 11:03:47 PDT 2013
Bill,
Thumbs up modulo the below comments.
I've noticed a couple of more minor issues.
Could you, please, make one more clean-up?
src/os/windows/vm/os_windows.cpp
There is still some inconsistency in the lines:
5401 // Builds a platform dependent Agent_OnLoad_<libname> function name
5411 char* os::build_agent_function_name(const char *sym_name, const char *lib_name,
5454 // agent_entry_name == _Agent_OnLoad_libName at XX
I'm not sure how to make it better but leave it up to you.
Maybe the line 5454 is Ok as another style is required there.
src/share/vm/prims/jvmti.xml
The lines are still too long: 667, 10773, 10776
Thanks,
Serguei
On 8/20/13 6:18 PM, BILL PITTORE wrote:
> 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