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

BILL PITTORE bill.pittore at oracle.com
Mon Nov 26 09:45:09 PST 2012


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