RFR - Changes to address CCC 8014135: Support for statically linked agents
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Aug 23 12:55:46 PDT 2013
Hi Bill,
This last version looks good to me. I went through my comments before
and you've addressed all of them. Thanks!
What testing did you run with these? Did you run linux/x64 nsk
vm.quick.testlist to see if nothing broke in the non-embedded
platforms? There are a lot of agent tests in this testset. I assume
you tested the static library configuration.
Thanks,
coleen
On 8/21/2013 3:33 PM, BILL PITTORE wrote:
> On 8/21/2013 2:03 PM, serguei.spitsyn at oracle.com wrote:
>> 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
>>
> All set, take a look at
> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.04/
>
> bill
>
>>
>> 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