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

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. 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.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
>>>>>>>>
>>>>>>>>
>>>>
>>>
>>>
>
>


More information about the hotspot-dev mailing list