RFR(M): 8247515: OSX pc_to_symbol() lookup does not work with core files

[Kevin Walls]

Sure -- I was thinking lowest_offset_from_sym initialising starting at a 
high positive integer (that would now be PTRDIFF_MAX I think) to save a 
comparison with e.g. -1, you can just check if the new offset is less 
than lowest_offset_from_sym

With the ptrdiff_t change you made, this all looks good to me however 
you decide. 8-)



On 07/07/2020 21:17, Chris Plummer wrote:
> Hi Kevin,
>
> Thanks for the review. Yes, that lack of initialization of 
> lowest_offset_from_sym is a bug. I'm real surprised the compiler 
> didn't catch it as it will be uninitialized garbage the first time it 
> is referenced. Fortunately usually the eventual offset is very small 
> if not 0, so probably this never prevented a proper match. I think 
> there's also another bug:
>
>  415       uintptr_t offset_from_sym = offset - sym->offset;
>
> "offset" is the passed in offset, essentially the address of the 
> symbol we are interested in, but given as an offset from the start of 
> the DSO. "sym->offset" is also an offset from the start of the DSO. It 
> could be located before or after "offset". This means the math could 
> result in a negative number, which when converted to unsigned would be 
> a very large positive number. This happens whenever you check a symbol 
> that is actually located after the address you are looking up. The end 
> result is harmless, because it just means there's no way we will match 
> that symbol, which is what you want, but it would be good to clean 
> this up.
>
> I think what is best is to use ptrdiff_t and initialize 
> lowest_offset_from_sym to -1. I've updated the webrev:
>
> http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/index.html
>
> thanks,
>
> Chris
>
> On 7/7/20 4:09 AM, Kevin Walls wrote:
>> Hi Chris,
>>
>> Yes I think this looks good.
>>
>> Question: In nearest_symbol, do we need to initialize 
>> lowest_offset_from_sym to something impossibly high, as if it 
>> defaults to zero we never find a better/nearer result?
>>
>> Thanks
>> Kevin
>>
>>
>> On 07/07/2020 06:10, Chris Plummer wrote:
>>> Hello,
>>>
>>> Please help review the following:
>>>
>>> http://cr.openjdk.java.net/~cjplummer/8247515/webrev.00/index.html
>>> https://bugs.openjdk.java.net/browse/JDK-8247515
>>>
>>> The CR contains a description of the issues being addressed. There 
>>> is also no test for this symbol lookup support yet. It will be there 
>>> after I push JDK-8247516 and  JDK-8247514, which are both blocked by 
>>> the CR.
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8247516
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8247514
>>>
>>> thanks,
>>>
>>> Chris
>>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200708/d933c759/attachment.htm>