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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.