<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Thanks Chris, yes looks good, I like that we check the library
bounds before calling nearest_symbol.</p>
<p>--<br>
Kevin</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 21/07/2020 21:05, Chris Plummer
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4c09d200-057b-e878-733a-524860453288@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Serguei and Kevin,<br>
<br>
The webrev has been updated:<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8247515/webrev.02/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8247515/webrev.02/index.html</a><br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8247515"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8247515</a><br>
<br>
Two issues were addressed:<br>
<br>
(1) Code in symbol_for_pc() assumed the caller had first checked
to make sure that the symbol is in a library, where-as some
callers assume NULL will be returned if the symbol is not in a
library. This is the case for pstack for example, which first
blindly does a pc to symbol lookup, and only if that returns
null does it then check if the pc is in the codecache or
interpreter. The logic in symbol_for_pc() assumed that if the pc
was greater than the start address of the last library in the
list, then it must be in that library. So in stack traces the
frames for compiled or interpreted pc's showed up as the last
symbol in the last library, plus some very large offset. The fix
is to now track the size of libraries so we can do a proper
range check.<br>
<br>
(2) There are issues with finding system libraries. See [1]
JDK-8249779. Because of this I disabled support for trying to
locate them. This was done in ps_core.c, and there are
"JDK-8249779" comment references in the code where I did this.
The end result of this is that PMap for core files won't show
system libraries, and PStack for core files won't show symbols
for addresses in system libraries. Note that currently support
for PMap and PStack is disabled for OSX, but I will shortly send
out a review to enable them for OSX core files as part of the
fix for [2] JDK-8248882.<br>
<br>
[1] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8249779"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8249779</a><br>
[2] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8248882"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8248882</a><br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 7/14/20 5:46 PM, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1e9e4786-79f8-c4e0-efe7-f1a8f369d2c6@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=UTF-8">
<div class="moz-cite-prefix">Okay, I'll wait for new webrev
version to review.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 7/14/20 17:40, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:bfb8c7e4-5e3f-81e7-0f20-ccad00f449dd@oracle.com">
<div class="moz-cite-prefix">Hi Serguie,<br>
<br>
Thanks for reviewing. This webrev is in limbo right now as I
discovered some issues that Kevin and I have been discussing
off line. One is that the code assumes the caller has first
checked to make sure that the symbol is in a library,
where-as the actual callers assume NULL will be returned if
the symbol is not in a library. The end result is that we
end up returning a symbol, even for address in the code
cache or interpreter. So in stack traces these frame show up
as the last symbol in the last library, plus some very large
offset. I have a fix for that were now I track the size of
each library. But there is another issue with the code that
tries to discover all libraries in the core file. It misses
a very large number of system libraries. We understand why,
but are not sure of the solution. I might just change to
code to only worry about user libraries (JDK libs and other
JNI libs).<br>
<br>
Some comments below also.<br>
<br>
On 7/14/20 4:37 PM, <a class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:18f7a1d8-3802-c7da-c981-eac8cf0953eb@oracle.com">
<div class="moz-cite-prefix">Hi Chris,<br>
<br>
I like the suggestion from Kevin below.<br>
</div>
</blockquote>
I'm not sure which suggestion since I updated the webrev based
on his initial suggestion.<br>
<blockquote type="cite"
cite="mid:18f7a1d8-3802-c7da-c981-eac8cf0953eb@oracle.com">
<div class="moz-cite-prefix"> <br>
I have a couple of minor comments so far.<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/libproc_impl.c.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/libproc_impl.c.frames.html</a><br>
<pre><span class="new"> 313 if (!lib->next || lib->next->base >= addr) {</span></pre>
I wonder if the check above has to be: <br>
<pre><span class="new"> 313 if (!lib->next || lib->next->base > addr) {</span></pre>
</div>
</blockquote>
Yes, > would be better, although this goes away with my fix
that tracks the size of each library.<br>
<blockquote type="cite"
cite="mid:18f7a1d8-3802-c7da-c981-eac8cf0953eb@oracle.com">
<div class="moz-cite-prefix"> <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/symtab.c.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/src/jdk.hotspot.agent/macosx/native/libsaproc/symtab.c.frames.html</a><br>
<pre><span class="changed"> 417 if (offset_from_sym >= 0) { // ignore symbols that comes after "offset"</span></pre>
Replace: comes => come<br>
</div>
</blockquote>
Ok.<br>
<br>
thanks,<br>
<br>
Chris<br>
<blockquote type="cite"
cite="mid:18f7a1d8-3802-c7da-c981-eac8cf0953eb@oracle.com">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 7/8/20 03:23, Kevin Walls wrote:<br>
</div>
<blockquote type="cite"
cite="mid:3c31622b-831e-d46f-daa6-d641cbf755b1@oracle.com">
<p><br>
</p>
<p>Sure -- I was thinking lowest_offset_from_sym
initialising starting at a high positive integer (that
would now be <span>PTRDIFF_MAX I think</span>) to save
a comparison with e.g. -1, you can just check if the new
offset is less than lowest_offset_from_sym</p>
<p>With the ptrdiff_t change you made, this all looks good
to me however you decide. 8-)<br>
</p>
<p><br>
</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 07/07/2020 21:17, Chris
Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:38e1887e-a646-abc0-0b7e-53339af1d684@oracle.com">Hi
Kevin, <br>
<br>
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: <br>
<br>
415 uintptr_t offset_from_sym = offset -
sym->offset; <br>
<br>
"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. <br>
<br>
I think what is best is to use ptrdiff_t and initialize
lowest_offset_from_sym to -1. I've updated the webrev: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8247515/webrev.01/index.html</a>
<br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 7/7/20 4:09 AM, Kevin Walls wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
Yes I think this looks good. <br>
<br>
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? <br>
<br>
Thanks <br>
Kevin <br>
<br>
<br>
On 07/07/2020 06:10, Chris Plummer wrote: <br>
<blockquote type="cite">Hello, <br>
<br>
Please help review the following: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8247515/webrev.00/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8247515/webrev.00/index.html</a>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8247515"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8247515</a>
<br>
<br>
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. <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8247516"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8247516</a>
<br>
[2] <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8247514"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8247514</a>
<br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>