Official reviewer needed: Review request for fix for 7200297: agent code does not handle multiple dll_dir paths correctly (original) (raw)

David Holmes david.holmes at oracle.com
Tue Nov 27 18:43:22 PST 2012

Thanks Bill - Reviewed.

David

On 28/11/2012 1:49 AM, BILL PITTORE wrote:

Thanks David for the review.

On 11/26/2012 9:38 PM, David Holmes wrote: Hi Bill,

A few minor comments, some of which you've touched on with Dmitry. David ------

share/back/debugInit.c 40 #include "sys.h" Shouldn't that be <sys.h> ? No, it's really "sys.h" -> jdk/src/share/back/export/sys.h: --- src/share/back/transport.c * Note: Java property sun.boot.library.path contains a single directory. + * Note: Above incorrect since 6819213 fixed. Dlldir is the first entry + * and -Dsun.boot.library.path entries are appended. Better to just change the original comment than to keep it and say it isn't true. Fixed. --- src/solaris/back/linkermd.c 113 return; Adding the return is superfluous. Arguably the whole method should be a chained if-else with no returns. Stylistically you have now mixed styles: either use a return, or use an else, but not both. Removed the return. --- src/solaris/demo/jvmti/hprof/hprofmd.c 426 return; Same comment as for linkermd.c And why didn't you move *holder = '\0'; in this version? Fixed both issues. Ditto src/windows/demo/jvmti/hprof/hprofmd.c Fixed. --- src/windows/back/linkermd.c 123 return; Ditto previous comments. Fixed. Updated webrev http://cr.openjdk.java.net/~bpittore/7200297/webrev.04/ Running nsk tests. bill --- On 27/11/2012 3:45 AM, BILL PITTORE wrote: Have a couple of reviews but still need official reviewer to pass muster. thanks, bill

On 11/22/2012 7:51 AM, Dmitry Samersoff wrote: Bill, Looks good for me. -Dmitry On 2012-11-21 23:31, BILL PITTORE wrote: Hi Dmitry, On 11/21/2012 4:56 AM, Dmitry Samersoff wrote: Bill,

Few nits: 1. dllbuildname is exactly the same for all locations could we place it to some common place? It looked somewhat intentional to have the agents and the hprof code be self-contained. I looked at using a common file but the makefile changes and source tree changes seemed a bit much. Hprof code is "unsupported demo" code and jdwp is supported agent so I went with the current scheme of having the code be self-contained. Also fileexists is not necessary and could be replaced with just ::access(buffer,ROK) (Windows has it as well) I'll go with the access suggestion above, less code. but if you prefer to keep it: strlen(filename)==0 could be a *filename == 0 2. We don't need else after return in all *md.c files e.g. linkermd.c:122 Semantically, you're correct. I think in terms of code readability I like the 'else' since it makes it clear to the reader that there are two different cases. C compiler will 'do the right thing'. Updated the webrev at http://cr.openjdk.java.net/~bpittore/7200297/webrev.03/ thanks, bill Otherwise looks good. -Dmitry On 2012-11-20 01:10, BILL PITTORE wrote: Have gotten reviews from Serguei Spitsyn for the changes, made some improvements and need an official reviewer to check it out. http://cr.openjdk.java.net/~bpittore/7200297/webrev.02 thanks, bill

On 11/14/2012 5:27 PM, bill.pittore at oracle.com wrote: This bug has to do with the jdwp and hprof agents not parsing the sun.boot.library.path (dlldir) correctly since the fix for 6819213 went in some years ago. This bug popped up while working on a particular platform that does not have the ability to set LDLIBRARYPATH before running the VM. As documented in the bug, on most platforms even if the sun.boot.library.path consists of multiple paths and the jdwp or hprof code fails to load a dependent lib, the system falls back to using LDLIBRARYPATH so the failure is masked. On some other platforms, this failover doesn't exist so we get an error when trying to load jdwp and hprof dependent libs. Some notes on a couple of files. * debugInit.c, hprofinit.c*: Rearranged the init order so that the jvmti pointer gets initialized before attempting to load the npt shared library. linkermd.c, hprofmd.c Much of the platform code in hprof and jdwp is duplicated and this change is no different. Based on the code in hotspot ossolaris/windows.cpp it parses the boot library path and attempts to find the library. * errormessages.c* Fixed a bug in the error message code that blindly dereferenced the npt pointer even if it wasn't set because of an error in loading. Webrev: http://cr.openjdk.java.net/~bpittore/7200297/webrev.00/ Ran the ute nsk/jdwp nsk/jvmti nsk/hprof tests, the JCK jdwp/jvmti tests and some command line testing on windows and linux. thanks, bill

More information about the hotspot-dev mailing list