RFR - Changes to address 8024007, minor fixes for static agent code
BILL PITTORE
bill.pittore at oracle.com
Wed Sep 11 09:18:58 PDT 2013
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/
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