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
