<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">On 8/4/20 11:04, Chris Plummer wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2cbb7908-7af1-6c18-7b53-7c943bdfcc7e@oracle.com">
<div class="moz-cite-prefix">On 8/4/20 2:11 AM, <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:2ca95f3f-c47f-d2c9-ab4f-347f65e74d9f@oracle.com">
<div class="moz-cite-prefix">Hi Yasumasa,<br>
<br>
The fix looks good to me.<br>
Thanks to Chris for discussing the details in previous emails.<br>
<br>
Just one suggestion:<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.02/src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.02/src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c.frames.html</a><br>
<pre><span class="new"> 44 #ifdef PF_R</span>
<span class="new"> 45 #define MAP_R_FLAG PF_R</span>
<span class="new"> 46 #else</span>
<span class="new"> 47 #define MAP_R_FLAG 0</span>
<span class="new"> 48 #endif</span></pre>
Could you, please add a small comment before?<br>
Something like this would be enough, I think:<br>
// Define a segment permission flag allowing read.<br>
<br>
</div>
</blockquote>
That comment is a little bit misleading, since on OSX the flag
really has no meaning. Maybe the following would be better:<br>
<br>
// Define a segment permission flag allowing read if there is a
read flag. Otherwise use 0.<br>
</blockquote>
<br>
Thanks, this is more precise.<br>
In fact, I was more concerned to explain what the <span class="new">macro
PF_R is about.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
</span>
<blockquote type="cite"
cite="mid:2cbb7908-7af1-6c18-7b53-7c943bdfcc7e@oracle.com"> <br>
thanks,<br>
<br>
Chris<br>
<blockquote type="cite"
cite="mid:2ca95f3f-c47f-d2c9-ab4f-347f65e74d9f@oracle.com">
<div class="moz-cite-prefix"> Thanks,<br>
Serguei<br>
<br>
<br>
On 8/3/20 17:54, Yasumasa Suenaga wrote:<br>
</div>
<blockquote type="cite"
cite="mid:29af768a-bebe-13fe-ac51-3062ede936bf@oss.nttdata.com">Thanks
Chris! <br>
I will push it when I got second reviewer. <br>
<br>
<br>
Yasumasa <br>
<br>
<br>
On 2020/08/04 9:54, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Yasumasa, <br>
<br>
Your changes look good now. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 8/3/20 5:47 PM, Yasumasa Suenaga wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
Thank you for the comment! <br>
I updated webrev: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.02/</a>
<br>
Diff from webrev.01: <a class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk/submit/rev/e98dc25b69c2"
moz-do-not-send="true">http://hg.openjdk.java.net/jdk/submit/rev/e98dc25b69c2</a>
<br>
<br>
On 2020/08/04 6:41, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Yasumasa, <br>
<br>
Your updated fix resulted in using the core file map
whereas the original fix used the library map. In both
cases the assert is avoided, which I think is the main
goal. Does it matter which map is used? <br>
</blockquote>
<br>
In GraalVM, read only segment is conflicted, thus it does
not matter which map is used. <br>
However this webrev is more generalize, so segments in
coredump should be used. <br>
<br>
<blockquote type="cite"> 42 #ifndef PF_R <br>
43 #define PF_R 0x4 <br>
44 #endif <br>
<br>
156 if ((map =
allocate_init_map(ph->core->classes_jsa_fd, <br>
157 offset, vaddr,
memsz, PF_R)) == NULL) { <br>
<br>
I'm not so sure this is appropriate for OSX. It uses
mach-o files, not elf files. The segment_command flags
field comes from loader.h [1]. I don't see anything in
there that looks like the equivalent of ELF access
flags. <br>
<br>
/* Constants for the flags field of the segment_command
*/ <br>
#define SG_HIGHVM 0x1 /* the file contents for
this segment is for <br>
the high part of the VM space, the
low part <br>
is zero filled (for stacks in core
files) */ <br>
#define SG_FVMLIB 0x2 /* this segment is the VM
that is allocated by <br>
a fixed VM library, for overlap
checking in <br>
the link editor */ <br>
#define SG_NORELOC 0x4 /* this segment has
nothing that was relocated <br>
in it and nothing relocated to it,
that is <br>
it maybe safely replaced without
relocation*/ <br>
#define SG_PROTECTED_VERSION_1 0x8 /* This segment is
protected. If the <br>
segment starts at file offset 0,
the <br>
first page of the segment is not
<br>
protected. All other pages of
the <br>
segment are protected. */ <br>
<br>
Since the flags don't matter for OSX, maybe you should
just pass 0. You can do something like: <br>
<br>
#ifndef PF_R <br>
#define MAP_R_FLAG PF_R <br>
#else <br>
#define MAP_R_FLAG 0 <br>
#endif <br>
</blockquote>
<br>
Thanks! <br>
I thought PF_R can be used PF_R from elf.h on macOS: <br>
<a class="moz-txt-link-freetext"
href="https://opensource.apple.com/source/dtrace/dtrace-90/sys/elf.h"
moz-do-not-send="true">https://opensource.apple.com/source/dtrace/dtrace-90/sys/elf.h</a>
<br>
<br>
I merged your code in this webrev. <br>
<br>
<blockquote type="cite">Some minor comment fixes are
needed: <br>
<br>
397 // Access flags fot this memory region is
different between the library <br>
<br>
"fot" -> "for" <br>
"is" -> "are" <br>
<br>
399 // We should respect to coredump. <br>
<br>
"to" -> "the" <br>
<br>
404 // And head of ELF header might be
included in coredump (See JDK-7133122). <br>
405 // Thus we need to replace PT_LOAD
segments the library version. <br>
<br>
How about: <br>
<br>
404 // Also the first page of the ELF header
might be included in the coredump (See JDK-7133122). <br>
405 // Thus we need to replace the PT_LOAD
segment with the library version. <br>
</blockquote>
<br>
Fixed them. <br>
<br>
<br>
Thanks, <br>
<br>
Yasumasa <br>
<br>
<br>
<blockquote type="cite">thanks, <br>
<br>
Chris <br>
<br>
[1] <a class="moz-txt-link-freetext"
href="https://opensource.apple.com/source/xnu/xnu-1456.1.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html"
moz-do-not-send="true">https://opensource.apple.com/source/xnu/xnu-1456.1.26/EXTERNAL_HEADERS/mach-o/loader.h.auto.html</a><br>
<br>
On 8/2/20 12:18 AM, Yasumasa Suenaga wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
(Remove "trivial" from subject) <br>
<br>
Thanks for the information! I fixed errors in new
webrev. It passed tests on submit repo
(mach5-one-ysuenaga-JDK-8250826-1-20200802-0151-13109525)
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.01/</a>
<br>
<br>
<br>
I tried to use elf.h instead of #define for PF_R,
however it failed
(mach5-one-ysuenaga-JDK-8250826-1-20200802-0542-13111335).
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk/submit/rev/67baee1a1a1d"
moz-do-not-send="true">http://hg.openjdk.java.net/jdk/submit/rev/67baee1a1a1d</a>
<br>
<br>
Thus I added #define for it in this webrev. <br>
<br>
<br>
Thanks, <br>
<br>
Yasumasa <br>
<br>
<br>
On 2020/08/02 10:22, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Yasumasa, <br>
<br>
[2020-08-01T14:15:42,514Z] Creating
support/native/jdk.hotspot.agent/libsaproc/static/libsaproc.a
from 8 file(s) <br>
[2020-08-01T14:15:43,961Z]
./open/src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c:128:8:
error: no member named 'flags' in 'struct map_info'
<br>
[2020-08-01T14:15:43,961Z] map->flags = flags;
<br>
[2020-08-01T14:15:43,961Z] ~~~ ^ <br>
[2020-08-01T14:15:43,963Z]
./open/src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c:153:54:
error: use of undeclared identifier 'PF_R' <br>
[2020-08-01T14:15:43,963Z] offset, vaddr, memsz,
PF_R)) == NULL) { <br>
<br>
I'll look at the code changes later. No time at the
moment. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
2020-08-01-1405571.suenaga.source2020-08-01-1405571.suenaga.source
2020-08-01-1405571.suenaga.source On 8/1/20 5:20 PM,
Yasumasa Suenaga wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
Thanks for your comment! <br>
I pushed new change to submit repo, but the build
failed on macOS. Could you share details? <br>
(I do not have Mac) <br>
<br>
commit: <a class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk/submit/rev/0eb1c497f297"
moz-do-not-send="true">http://hg.openjdk.java.net/jdk/submit/rev/0eb1c497f297</a>
<br>
job:
mach5-one-ysuenaga-JDK-8250826-1-20200801-1407-13098989
<br>
<br>
On 2020/08/01 13:06, Chris Plummer wrote: <br>
<blockquote type="cite">On 7/30/20 6:18 PM,
Yasumasa Suenaga wrote: <br>
<blockquote type="cite">Hi Chris, <br>
<br>
On 2020/07/31 7:29, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Yasumasa, <br>
<br>
If I understand correctly we first call
add_map_info() for all the PT_LOAD segments
in the core file. We then process all the
library segments, calling add_map_info() for
them if the target_vaddr has not already
been addded. If has already been added,
which I assume is the case for any library
segment that is already in the core file,
then the core file version is replaced the
the library version. I'm a little unclear
of the purpose of this replacing of the core
PT_LOAD segments with those found in the
libraries. If you could explain this that
would help me understand your change. <br>
</blockquote>
<br>
Read only segments in ELF should not be any
different from PT_LOAD segments in the core. <br>
And head of ELF header might be included in
coredump (See JDK-7133122). Thus we need to
replace PT_LOAD segments the library version.
<br>
</blockquote>
Ok. The code in the area really should have been
commented better when first written. The purpose
is not understandable simply by reading the
code. <br>
</blockquote>
<br>
I added some comments to existing code. Please
tell me if it is insufficient. <br>
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite">I'm also unsure why
existing_map->fd would ever be something
other than the core file. Why would another
library map the same target_vaddr. <br>
</blockquote>
<br>
When mmap() is called to read-only ELF
segments / sections, Linux kernel seems to
allocate other memory segments which has same
top virtual memory address. I've not yet found
out from the code of Linux kernel, but I
confirmed this behavior on GDB. <br>
</blockquote>
Ok. Same comment as above. This should have been
explained with comments in the code. <br>
</blockquote>
<br>
Added some comments. <br>
<br>
<br>
<blockquote type="cite">As for your fix, if I
understand correctly the issue is that a single
segment in the library is being split into two
segments in the process (and therefore in the
core file) due to an mprotect being done on part
of the segment. Because of this the segment size
in the library does match the segment size in
the core file. So with your fix the library
segment is used, but what about the other half
of the segment that is in the core file? Don't
we now have overlapping segments; the full
original segment from the library, and then a
second segment that overlaps the tail end of the
library segment? Will that cause any confusion
later on? <br>
</blockquote>
<br>
As long as vaddr is valid, it doesn't matter even
if it overlaps because SA would sort the map with
vaddr, and would lookup with it. <br>
In Substrate VM, there are RO and RW sections in
that order, so it is ok with webrev.00 . However
it might not be appropriate because RW section
might be top of PT_LOAD. <br>
<br>
To make it more generalized, I changed it to the
commit on submit repo. <br>
It would check access flags between in coredump
and in binary. If they are different, we respect
current (loaded from coredump) map because it
might be changed at runtime. <br>
<br>
The change for LabsJDK 11 is more simple because
JDK 11 does not have ps_core_common.c . <br>
So I share you it. It may help you: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/JDK-8250826-labsjdk11-0.patch"
moz-do-not-send="true">http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/JDK-8250826-labsjdk11-0.patch</a>
<br>
<br>
<br>
Thanks, <br>
<br>
Yasumasa <br>
<br>
<br>
<blockquote type="cite">thanks, <br>
<br>
Chris <br>
<blockquote type="cite"> <br>
<br>
Thanks, <br>
<br>
Yasumasa <br>
<br>
<br>
<blockquote type="cite">thanks, <br>
<br>
Chris <br>
<br>
On 7/30/20 1:18 PM, Chris Plummer wrote: <br>
<blockquote type="cite">Hi Yasumasa, <br>
<br>
I'm reviewing this RFR, and I'd like to
ask that it not be pushed as trivial.
Although it is just a one line change, it
takes an extensive knowledge to understand
the impact. I'll read up on the filed
graal issue and try to understand the ELF
code a bit better. <br>
<br>
thanks, <br>
<br>
Chris <br>
<br>
On 7/30/20 6:45 AM, Yasumasa Suenaga
wrote: <br>
<blockquote type="cite">Hi all, <br>
<br>
Please review this trivial change: <br>
<br>
JBS: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8250826"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8250826</a>
<br>
webrev: <a
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~ysuenaga/JDK-8250826/webrev.00/</a>
<br>
<br>
I played Truffle NFI on GraalVM, but I
cannot get Java stacks from coredump via
jhsdb. <br>
<br>
I've reported this issue to GraalVM
community [1], and I 've found out the
cause of this issue is .svm_heap would
be separated to RO and RW areas by
mprotect() calls in run time in spite of
.svm_heap is RO section in ELF (please
see [1] for details). <br>
<br>
It is corner case, but we will see same
problem on jhsdb when we attempt to
analyze coredump which comes from some
applications / libraries which would
separate RO sections in ELF like
Substrate VM. <br>
<br>
I sent PR to fix libsaproc.so in LabsJDK
11 for this issue [2], then community
members suggested me to discuss in
serviceability-dev. <br>
<br>
<br>
Thanks, <br>
<br>
Yasumasa <br>
<br>
<br>
[1] <a class="moz-txt-link-freetext"
href="https://github.com/oracle/graal/issues/2579"
moz-do-not-send="true">https://github.com/oracle/graal/issues/2579</a>
<br>
[2] <a class="moz-txt-link-freetext"
href="https://github.com/graalvm/labs-openjdk-11/pull/9"
moz-do-not-send="true">https://github.com/graalvm/labs-openjdk-11/pull/9</a>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>