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

[Daniel D. Daugherty]

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:')
>>
>
>