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

[BILL PITTORE]

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