Looks good.
One more reviewer is needed, right?
Thanks,
Serguei
On 11/16/12 12:26 PM, BILL PITTORE wrote:
On 11/15/2012 2:49 PM, serguei.spit...@oracle.com wrote:
Bill,
It looks good.
Minor:
The line "int n;" is not needed and can be deleted in the files:
||*src/solaris/back/linker_md.c*
||*src/windows/demo/jvmti/hprof/hprof_md.c*
* src/windows/back/linker_md.c*
*src/windows/demo/jvmti/hprof/hprof_md.c*
||*
*
Oops. Missed that one. Fixed now in webrev.02
**Similar simplification can be done:
* src/windows/demo/jvmti/hprof/hprof_md.c*
Did that but did not save file to disk before running webrev.
I do not have a reviewer status, but you can list as a reviewer. :)
Thank you for fixing it!
Ok, will do.
thanks,
bill
Thanks,
Serguei
On 11/15/12 11:27 AM, BILL PITTORE wrote:
Hi Serguei,
Thanks for the review and your excellent suggestion at some
simplification in dll_build_name. Applied the changes, built, test
and all is still working. New webrev at:
http://cr.openjdk.java.net/~bpittore/7200297/webrev.01/
Are you an official reviewer?
thanks,
bill
On 11/15/2012 4:00 AM, serguei.spit...@oracle.com wrote:
||Hi Bill,
*src/share/back/debugInit.c*
**271 JVMTI_FUNC_PTR(gdata->jvmti,GetSystemProperty)
Space is missed after comma.
||*src/share/back/error_messages.c
*
74 /* May be called before NPT is initialized so don't fault */
The comment seems to belong to the else statement.
||*src/share/back/transport.c
*||*src/share/demo/jvmti/hprof/hprof.h*||*
src/share/demo/jvmti/hprof/hprof_init.c*
No comments.
||*src/solaris/back/linker_md.c
*
***Lines 75-96:*
It seems the *if/else* is not needed as the while loop would
serve the else as well.
* 73 int n;*
Line #73 can be removed as "n" is not used.
* 81 if ((p - pathname) == 0) {*
Nit: Replace *(p - pathname) == 0* with *p == pathname*
* 119 char *p;*
Line #119 can be removed as "p" is not used.
||*src/solaris/demo/jvmti/hprof/hprof_md.c
*
***Lines 395-418:*
The same comments as for the *linker_md.c*.
||*src/solaris/npt/npt_md.h
*
No comments.
||*src/windows/back/linker_md.c
*
The same comments as for the *linker_md.c*.
||*src/windows/demo/jvmti/hprof/hprof_md.c
*
****Lines 381-407:*
*
The same comments as for the *linker_md.c*.
*
*
||*src/windows/npt/npt_md.h
*
No comments.
Thanks,
Serguei
On 11/14/12 2: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
