Review request for fix for 7200297
BILL PITTORE
bill.pittore at oracle.com
Fri Nov 16 12:26:52 PST 2012
On 11/15/2012 2:49 PM, serguei.spitsyn at 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.spitsyn at 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.pittore at 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
>>>>
>>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121116/db28c70d/attachment.html
More information about the serviceability-dev
mailing list