Need Second Reviewer Re: RR(S): JDK-7127191 SA JSDB does not display native symbols correctly for transported Linux cores

[serguei.spitsyn at oracle.com]

Dmitry,

It looks good.
Thank you for answers and code adjustment.

Thanks,
Serguei


On 1/31/14 1:48 AM, Dmitry Samersoff wrote:
> Serguei,
>
> Thank you for review. I'd updated webrev (see also below)
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.03/
>
>
> On 2014-01-31 04:01, serguei.spitsyn at oracle.com wrote:
>> Hi Dmitry,
>>
>>
>> agent/src/os/linux/libproc_impl.c
>>
>> Q: To the lines 56-71 (I'm taking a blame for asking stupid questions):
>>       Is it possible the name argument has no leading slash '/' ?
>>       Does it work correctly in such a case?
>>       Is there a need to add a slash after the alt_path?
> Linker always put full path to solib image to process .DYNSECTION. The
> only exclusion - couple of pseudo-objects providing interface to kernel
> like linux-gate.so etc, but SA doesn't handle it.
>
> I put appropriate comment.
>
>> agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java
>>
>> Something is wrong with the comment as it is not clear:
>>
>>    59     /* Typically we don't have too much loaded object here,
>>    60        so final efforts to do a linear search less then sort and do
>>    61        binary serach */
>>
>> Did you want to say:  "too many objects" or "to match loaded object" ?
>> And what does it mean:
>>    "so final efforts to do a linear search less then sort and do binary
>> serach" ?
>>
>> Did you want to say something like:
>>    "do a linear search instead of binary search"
> Comments rephrased.
>
>> agent/src/share/classes/sun/jvm/hotspot/utilities/soql/sa.js
>>
>>    nit: Remove redundant else at line 896, it saves the extra indent.
> I`m itching to reformat this function to make it readable since first
> time i saw it.
>
> You gives me enough courage to do it ;) So see webrev.
>
>
>>    nit: More simplifications to consider:
>>
>>      function closestSymbolFor(addr) {
>>         var dso = (sa.cdbg == null) ? null :
>> sa.cdbg.loadObjectContainingPC(addr);
>>         return (dso == null) ? null : dso.closestSymbolToPC(addr);
>>      }
>>
>>      function loadObjectContainingPC(addr) {
>>          // sa.cdbg == null: no CDebugger support, return null
>>          return (sa.cdbg == null) ? null :
>> sa.cdbg.loadObjectContainingPC(addr);
>>      }
> I would prefer to keep full if for readability.
>
> -Dmitry
>
>
>>
>> Thanks,
>> Serguei
>>
>> On 1/29/14 6:17 AM, Dmitry Samersoff wrote:
>>> Hi Everyone,
>>>
>>> Please review the fix.
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.02/
>>>
>>> -Dmitry
>>>
>>> On 2014-01-20 20:00, Dmitry Samersoff wrote:
>>>> Hi Everyone,
>>>>
>>>> Please review the fix.
>>>>
>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-7127191/webrev.01/
>>>>
>>>> This fix doesn't solve all problems with symbol lookup in SA, but
>>>> address the problem mentioned in bug reports.
>>>>
>>>> 1. I change algorithm of pathmap_open. Now, it tries to find library
>>>> hardly.
>>>>
>>>> 2. I decided not to fix broken binary search in loadObjectContainingPC,
>>>> because with less then 20 DSO's we typically have here performance of
>>>> just linear search is approx the same as load, sort, convert to array
>>>> and do binary search.
>>>>
>>>> -Dmitry
>>>>
>