[8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

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


Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

JC Beyler
Hi Severin,

The fix looks good to me.

For reference for other reviewers, the original change can be seen here:

And it contains fixes from the following change (which puts a +1 to avoid overflow):

from the bug:

Thanks,
Jc

On Fri, Nov 9, 2018 at 8:03 AM Severin Gehwolf <[hidden email]> wrote:
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,
Jc
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Severin Gehwolf
Hi JC,

On Fri, 2018-11-09 at 08:46 -0800, JC Beyler wrote:
> Hi Severin,
>
> The fix looks good to me.

Thanks for the review!

> 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 for these references. I should have put them into the review
email right from the start.

Cheers,
Severin

> Thanks,
> Jc
>
> On Fri, Nov 9, 2018 at 8:03 AM Severin Gehwolf <[hidden email]>
> wrote:
> > 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
> >
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Jini George
In reply to this post by Severin Gehwolf
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.

On 11/9/2018 9:31 PM, Severin Gehwolf wrote:

> 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
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Severin Gehwolf
On Tue, 2018-11-13 at 10:46 +0530, Jini George wrote:
> 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


> 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.
>
> On 11/9/2018 9:31 PM, Severin Gehwolf wrote:
> > 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
> >
> >

Reply | Threaded
Open this post in threaded view
|

Re: [PING] [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Severin Gehwolf
On Tue, 2018-11-13 at 09:28 +0100, Severin Gehwolf wrote:
> On Tue, 2018-11-13 at 10:46 +0530, Jini George wrote:
> > 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

Any JDK 8u Reviewer willing to OK this?

Thanks,
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.
> >
> > On 11/9/2018 9:31 PM, Severin Gehwolf wrote:
> > > 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
> > >
> > >

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Andrew Hughes-8
In reply to this post by Severin Gehwolf
On Fri, 9 Nov 2018 at 16:02, Severin Gehwolf <[hidden email]> wrote:

>
> 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
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Severin Gehwolf
On Mon, 2018-11-19 at 15:09 +0000, Andrew Hughes wrote:

> On Fri, 9 Nov 2018 at 16:02, Severin Gehwolf <[hidden email]> wrote:
> >
> > 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!

> 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

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Andrew Hughes-8
On Mon, 19 Nov 2018 at 15:39, Severin Gehwolf <[hidden email]> wrote:

snip...

>
> 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
Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Severin Gehwolf
On Wed, 2018-11-21 at 06:47 +0000, Andrew Hughes wrote:

> On Mon, 19 Nov 2018 at 15:39, Severin Gehwolf <[hidden email]> wrote:
>
> snip...
>
> >
> > 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

Nice! The JDK 12 change from 8210836 applies as-is (post-path-
shuffeling) with this dependency in place.

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: [8u] RFR: 8210836: Build fails with warn_unused_result in openjdk/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c

Andrew Hughes-8
On Wed, 21 Nov 2018 at 10:20, Severin Gehwolf <[hidden email]> wrote:

>
> On Wed, 2018-11-21 at 06:47 +0000, Andrew Hughes wrote:
> > On Mon, 19 Nov 2018 at 15:39, Severin Gehwolf <[hidden email]> wrote:
> >
> > snip...
> >
> > >
> > > 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
>
> 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