RFR - Changes to address 8024007, minor fixes for static agent code
BILL PITTORE
bill.pittore at oracle.com
Wed Sep 11 14:14:07 PDT 2013
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:')
>>>
>>
>>
>
More information about the hotspot-runtime-dev
mailing list