Discussion:
[8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c
Severin Gehwolf
2018-11-09 16:01:27 UTC
Permalink
Hi,

Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.

Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/

Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.

Thoughts?

Thanks,
Severin
JC Beyler
2018-11-09 16:46:08 UTC
Permalink
Hi Severin,

The fix looks good to me.

For reference for other reviewers, the original change can be seen here:
http://hg.openjdk.java.net/jdk/jdk/rev/c0f9161f591e

And it contains fixes from the following change (which puts a +1 to avoid
overflow):
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/cd86b5699825

from the bug:
https://bugs.openjdk.java.net/browse/JDK-8140482

Thanks,
Jc
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
--
Thanks,
Jc
Severin Gehwolf
2018-11-09 17:02:12 UTC
Permalink
Hi JC,
Post by JC Beyler
Hi Severin,
The fix looks good to me.
Thanks for the review!
Post by JC Beyler
http://hg.openjdk.java.net/jdk/jdk/rev/c0f9161f591e
And it contains fixes from the following change (which puts a +1 to
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/cd86b5699825
https://bugs.openjdk.java.net/browse/JDK-8140482
Thanks for these references. I should have put them into the review
email right from the start.

Cheers,
Severin
Post by JC Beyler
Thanks,
Jc
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux.
Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Jini George
2018-11-13 05:16:47 UTC
Permalink
Looks good to me, Severin.

While looking at this, I realized that the code improvement change of
8140482 in ps_core.c of increasing the BUF_SIZE by 1 and adding the '\0'
would be needed for MacOS also -- I will file another bug for that for
JDK 12.

Thanks!
Jini.
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Severin Gehwolf
2018-11-13 08:28:34 UTC
Permalink
Post by Jini George
Looks good to me, Severin.
Thanks for the review!

I believe I still need a review from a JDK 8u Reviewer:
http://openjdk.java.net/census#jdk8u

Thanks,
Severin
Post by Jini George
While looking at this, I realized that the code improvement change of
8140482 in ps_core.c of increasing the BUF_SIZE by 1 and adding the '\0'
would be needed for MacOS also -- I will file another bug for that for
JDK 12.
Thanks!
Jini.
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Severin Gehwolf
2018-11-16 10:35:31 UTC
Permalink
Post by Severin Gehwolf
Post by Jini George
Looks good to me, Severin.
Thanks for the review!
http://openjdk.java.net/census#jdk8u
Any JDK 8u Reviewer willing to OK this?

Thanks,
Severin
Post by Severin Gehwolf
Post by Jini George
While looking at this, I realized that the code improvement change of
8140482 in ps_core.c of increasing the BUF_SIZE by 1 and adding the '\0'
would be needed for MacOS also -- I will file another bug for that for
JDK 12.
Thanks!
Jini.
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Andrew Hughes
2018-11-19 15:09:36 UTC
Permalink
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Is there a reason not to just backport
https://bugs.openjdk.java.net/browse/JDK-8140482
as a pre-requisite for this? It looks pretty uncontroversial (a lot of
it is indenting fixes),
makes the code a little more secure and would help any future
backports in these areas
if the code is in sync. The resulting backport of 8210836 would then
be largely as is
and may not even require review.
--
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Severin Gehwolf
2018-11-19 15:39:18 UTC
Permalink
Post by Andrew Hughes
Post by Severin Gehwolf
Hi,
Could somebody please review this 8u backport of 8210836 as I'd like to
get 8210647 (opt for sa) backported to 8u as well? Unfortunately the
change from JDK 12 doesn't apply cleanly so I've included select
changes from 8140482 so that the backport remains minimal. If anything,
this makes the code more robust I'd think.
Bug: https://bugs.openjdk.java.net/browse/JDK-8210836
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210836/jdk8/webrev.01/
Testing: Manual testing of loading core file in SA on linux. Stepping
through code in the debugger. Basic jsadebugd core file loading.
Thoughts?
Thanks,
Severin
Thanks for the review!
Post by Andrew Hughes
Is there a reason not to just backport
https://bugs.openjdk.java.net/browse/JDK-8140482
as a pre-requisite for this? It looks pretty uncontroversial (a lot of
it is indenting fixes),
makes the code a little more secure and would help any future
backports in these areas
if the code is in sync.
Two reasons: (1) JDK-8140482 isn't a small change. (2) The patch
doesn't apply cleanly[1] from JDK 9 so would be some work to get the
pre-requisite in.

It appears the code is already out of sync and the risk would be higher
if we backported all of JDK-8140482 for a relatively small change of
JDK-8210836.

The aim was to keep the backport as minimal as possible.

Thanks,
Severin

[1] I'm getting this:

$ hg import ../../openjdk9-mainline/hotspot/JDK-8140482.jdk9.export.patch
applying ../../openjdk9-mainline/hotspot/JDK-8140482.jdk9.export.patch
patching file agent/src/os/linux/libproc_impl.c
Hunk #1 FAILED at 37
Hunk #2 FAILED at 47
Hunk #3 FAILED at 69
3 out of 3 hunks FAILED -- saving rejects to file agent/src/os/linux/libproc_impl.c.rej
patching file agent/src/os/linux/ps_core.c
Hunk #1 FAILED at 773
1 out of 1 hunks FAILED -- saving rejects to file agent/src/os/linux/ps_core.c.rej
patching file src/cpu/x86/vm/stubRoutines_x86.cpp
Hunk #1 FAILED at 146
1 out of 1 hunks FAILED -- saving rejects to file src/cpu/x86/vm/stubRoutines_x86.cpp.rej
unable to find 'src/cpu/x86/vm/templateTable_x86.cpp' for patching
(use '--prefix' to apply patch relative to the current directory)
2 out of 2 hunks FAILED -- saving rejects to file src/cpu/x86/vm/templateTable_x86.cpp.rej
patching file src/os/linux/vm/os_linux.cpp
Hunk #2 FAILED at 5928
1 out of 2 hunks FAILED -- saving rejects to file src/os/linux/vm/os_linux.cpp.rej
patching file src/share/vm/runtime/deoptimization.cpp
Hunk #2 FAILED at 2082
Hunk #3 FAILED at 2178
2 out of 3 hunks FAILED -- saving rejects to file src/share/vm/runtime/deoptimization.cpp.rej
patching file src/share/vm/runtime/task.cpp
Hunk #1 succeeded at 122 with fuzz 1 (offset 4 lines).
patching file src/share/vm/services/memoryService.cpp
Hunk #1 succeeded at 546 with fuzz 2 (offset -2 lines).
Hunk #2 FAILED at 558
1 out of 3 hunks FAILED -- saving rejects to file src/share/vm/services/memoryService.cpp.rej
abort: patch failed to apply
Andrew Hughes
2018-11-21 06:47:40 UTC
Permalink
On Mon, 19 Nov 2018 at 15:39, Severin Gehwolf <***@redhat.com> wrote:

snip...
Post by Severin Gehwolf
Two reasons: (1) JDK-8140482 isn't a small change. (2) The patch
doesn't apply cleanly[1] from JDK 9 so would be some work to get the
pre-requisite in.
It appears the code is already out of sync and the risk would be higher
if we backported all of JDK-8140482 for a relatively small change of
JDK-8210836.
The aim was to keep the backport as minimal as possible.
Not too bad. I've proposed it:

https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-November/035349.html
--
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Severin Gehwolf
2018-11-21 10:20:37 UTC
Permalink
Post by Andrew Hughes
snip...
Post by Severin Gehwolf
Two reasons: (1) JDK-8140482 isn't a small change. (2) The patch
doesn't apply cleanly[1] from JDK 9 so would be some work to get the
pre-requisite in.
It appears the code is already out of sync and the risk would be higher
if we backported all of JDK-8140482 for a relatively small change of
JDK-8210836.
The aim was to keep the backport as minimal as possible.
https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-November/035349.html
Nice! The JDK 12 change from 8210836 applies as-is (post-path-
shuffeling) with this dependency in place.

Thanks,
Severin
Andrew Hughes
2018-11-21 16:01:09 UTC
Permalink
Post by Severin Gehwolf
Post by Andrew Hughes
snip...
Post by Severin Gehwolf
Two reasons: (1) JDK-8140482 isn't a small change. (2) The patch
doesn't apply cleanly[1] from JDK 9 so would be some work to get the
pre-requisite in.
It appears the code is already out of sync and the risk would be higher
if we backported all of JDK-8140482 for a relatively small change of
JDK-8210836.
The aim was to keep the backport as minimal as possible.
https://mail.openjdk.java.net/pipermail/hotspot-dev/2018-November/035349.html
Nice! The JDK 12 change from 8210836 applies as-is (post-path-
shuffeling) with this dependency in place.
Thanks,
Severin
Ah, good! That's what I hoped. Then 8210836 should be just an approval.
--
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Loading...