RFR - Changes to address 8024007, minor fixes for static agent code

[BILL PITTORE]

Thanks for the review!

bill

On 9/11/2013 4:24 PM, Daniel D. Daugherty wrote:
> On 9/11/13 10:18 AM, BILL PITTORE wrote:
>> Dan had a few comments after I pushed the original code for static 
>> agent support, bugid 8014135. These changes address those comments. 
>> Webrev at:
>>
>> http://cr.openjdk.java.net/~bpittore/8024007/webrev.00/
>
>
> src/share/vm/prims/jvmti.xml
>     No comments.
>
> src/share/vm/runtime/arguments.hpp
>     No comments.
>
> src/share/vm/runtime/thread.cpp
>     No comments.
>
> src/share/vm/runtime/os.cpp
>     No comments.
>
> src/os/windows/vm/os_windows.cpp
>     No comments.
>
> Thanks for following up with the changes even though my review
> was so very late.
>
> Thumbs up!
>
> Dan
>
>
>>
>> thanks,
>>  bill
>>
>> On 8/29/2013 12:55 PM, Daniel D. Daugherty wrote:
>>> On 8/21/13 1:33 PM, BILL PITTORE wrote:
>>>> http://cr.openjdk.java.net/~bpittore/8014135/hotspot/webrev.04/
>>>
>>> Sorry for the very late review.
>>>
>>> Nothing critical or "must do" here.
>>>
>>> src/share/vm/prims/jvmti.xml
>>>     line 460: The VM will invoke the Agent_OnUnload_L function of 
>>> the agent, if such
>>>     line 461: a function is exported, at the same point during 
>>> startup as it would
>>>     line 462: have called the dynamic entry point Agent_OnUnLoad.
>>>
>>>         The "at the same point during startup" part caught my eye. Not
>>>         sure why you're constraining this to "during startup". Perhaps
>>>         "during VM execution" instead.
>>>
>>>         I'm having a little bit of difficulty groking the idea of 
>>> calling
>>>         an unload function when the statically linked library can 
>>> really
>>>         be unloaded. However, I can see the need for a hook of some 
>>> sort
>>>         that can be called when the library would be logically 
>>> unloaded.
>>>
>>> src/share/vm/runtime/arguments.hpp
>>>     line 147: void set_static_lib(bool static_lib)      { 
>>> _is_static_lib = static_lib; }
>>>
>>>         The usual HotSpot style would be:
>>>
>>>           void set_static_lib(bool is_static_lib)   { _is_static_lib 
>>> = is_static_lib; }
>>>
>>>         i.e., same name differentiated by a leading '_' only.
>>>
>>> src/share/vm/prims/jvmtiExport.cpp
>>>     No comments.
>>>
>>> src/share/vm/runtime/thread.cpp
>>>     line 3707: // First check to see if agent is statcally linked 
>>> into executable
>>>         typo: 'statcally' -> 'statically'
>>>
>>> src/share/vm/runtime/os.hpp
>>>     No comments.
>>>
>>> src/share/vm/runtime/os.cpp
>>>     line 455: void* os::find_agent_function(AgentLibrary *agent_lib, 
>>> bool check_lib,
>>>     line 456:                               const char *syms[], 
>>> size_t syms_len) {
>>>         assert(agent_lib != NULL, "sanity check");
>>>
>>>         would be a good addition.
>>>
>>>     line 481: bool os::find_builtin_agent(AgentLibrary *agent_lib, 
>>> const char *syms[],
>>>     line 482:                             size_t syms_len) {
>>>         assert(agent_lib != NULL, "sanity check");
>>>
>>>         would be a good addition.
>>>
>>>     This block:
>>>
>>>     495   ret = find_agent_function(agent_lib, true, syms, syms_len);
>>>     496   agent_lib->set_os_lib(save_handle);
>>>     497   if (ret != NULL) {
>>>     498     // Found an entry point like Agent_OnLoad_lib_name so we 
>>> have a static agent
>>>     499     agent_lib->set_os_lib(proc_handle);
>>>     500     agent_lib->set_valid();
>>>     501     agent_lib->set_static_lib(true);
>>>     502     return true;
>>>     503   }
>>>     504   return false;
>>>
>>>     would be more clear as:
>>>
>>>     495   ret = find_agent_function(agent_lib, true, syms, syms_len);
>>>     496   if (ret != NULL) {
>>>     497     // Found an entry point like Agent_OnLoad_lib_name so we 
>>> have a static agent
>>>     498     agent_lib->set_valid();
>>>     499     agent_lib->set_static_lib(true);
>>>     500     return true;
>>>     501   }
>>>     502   agent_lib->set_os_lib(save_handle);
>>>     503   return false;
>>>
>>>
>>> src/os/posix/vm/os_posix.cpp
>>>     No comments.
>>>
>>> src/os/windows/vm/os_windows.cpp
>>>     line 5427: // Need to check for C:
>>>         Need to check for drive prefix. (not just 'c:')
>>>
>>
>>
>