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. Dll_dir 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/linker_md.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/hprof_md.c
426 return;
Same comment as for linker_md.c
And why didn't you move *holder = '\0'; in this version?
Fixed both issues.
Ditto src/windows/demo/jvmti/hprof/hprof_md.c
Fixed.
---
src/windows/back/linker_md.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.
dll_build_name 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 file_exists is not necessary and could be
replaced with just ::access(buffer,R_OK) (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. linker_md.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.pitt...@oracle.com wrote:
This bug has to do with the jdwp and hprof agents not parsing the
sun.boot.library.path (dll_dir) 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
LD_LIBRARY_PATH 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 LD_LIBRARY_PATH 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, hprof_init.c*:
Rearranged the init order so that the jvmti pointer gets
initialized
before attempting to load the npt shared library.
*linker_md.c, hprof_md.c*
Much of the platform code in hprof and jdwp is duplicated and this
change is no different. Based on the code in hotspot
os_solaris/windows.cpp it parses the boot library path and
attempts to
find the library.
*
error_messages.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
