[10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

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

[10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

Thomas Schatzl
Hi all,

  can I have reviews for this small change that prevents C++ compilers
from generating code that reloads oop value from the heap instead of
using a local variable in concurrent marking closures. This can cause
issues if this reloading occurs after the originally loaded value has
been checked for NULL to prevent SIGSEGVs and the mutator changed the
value in the meantime.

E.g.

  oop o = load_from_heap(p);
  if (o == NULL) {
     return;
  }
  // do something with o; potential crash here when reloading from p
instead of actually reusing o

According to SAP particularly the IBM xlc compiler tends to to such
nasty things (which is completely okay from a language POV).

The fix is to make the load from the heap a volatile load using the new
AccessBarrier API in cases this can happen.

CR:
https://bugs.openjdk.java.net/browse/JDK-8129440
Webrev:
http://cr.openjdk.java.net/~tschatzl/8129440/webrev/
Testing:
hs tier1+2; note that this has only been reported for the IBM xlc
compiler for PPC which Oracle does not test for. I hope somebody at SAP
can confirm that it generates the correct code now.

Based on JDK-8193063 also out for review right now.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: [10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

Erik Österlund-2
Hi Thomas,

Looks good to me.

Thanks,
/Erik

On 2017-12-06 20:17, Thomas Schatzl wrote:

> Hi all,
>
>    can I have reviews for this small change that prevents C++ compilers
> from generating code that reloads oop value from the heap instead of
> using a local variable in concurrent marking closures. This can cause
> issues if this reloading occurs after the originally loaded value has
> been checked for NULL to prevent SIGSEGVs and the mutator changed the
> value in the meantime.
>
> E.g.
>
>    oop o = load_from_heap(p);
>    if (o == NULL) {
>       return;
>    }
>    // do something with o; potential crash here when reloading from p
> instead of actually reusing o
>
> According to SAP particularly the IBM xlc compiler tends to to such
> nasty things (which is completely okay from a language POV).
>
> The fix is to make the load from the heap a volatile load using the new
> AccessBarrier API in cases this can happen.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8129440
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8129440/webrev/
> Testing:
> hs tier1+2; note that this has only been reported for the IBM xlc
> compiler for PPC which Oracle does not test for. I hope somebody at SAP
> can confirm that it generates the correct code now.
>
> Based on JDK-8193063 also out for review right now.
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: [10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

Erik Helin-2
In reply to this post by Thomas Schatzl
On 12/06/2017 08:17 PM, Thomas Schatzl wrote:

> Hi all,
>
>    can I have reviews for this small change that prevents C++ compilers
> from generating code that reloads oop value from the heap instead of
> using a local variable in concurrent marking closures. This can cause
> issues if this reloading occurs after the originally loaded value has
> been checked for NULL to prevent SIGSEGVs and the mutator changed the
> value in the meantime.
>
> E.g.
>
>    oop o = load_from_heap(p);
>    if (o == NULL) {
>       return;
>    }
>    // do something with o; potential crash here when reloading from p
> instead of actually reusing o
>
> According to SAP particularly the IBM xlc compiler tends to to such
> nasty things (which is completely okay from a language POV).
>
> The fix is to make the load from the heap a volatile load using the new
> AccessBarrier API in cases this can happen.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8129440
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8129440/webrev/

Looks good, Reviewed.

Thanks,
Erik

> Testing:
> hs tier1+2; note that this has only been reported for the IBM xlc
> compiler for PPC which Oracle does not test for. I hope somebody at SAP
> can confirm that it generates the correct code now.
>
> Based on JDK-8193063 also out for review right now.
>
> Thanks,
>    Thomas
>
Reply | Threaded
Open this post in threaded view
|

Re: [10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

Thomas Schatzl
Hi,

On Thu, 2017-12-07 at 10:14 +0100, Erik Helin wrote:

> On 12/06/2017 08:17 PM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for this small change that prevents C++
> > compilers from generating code that reloads oop value from the heap
> > instead of using a local variable in concurrent marking closures.
> > This can cause issues if this reloading occurs after the originally
> > loaded value has been checked for NULL to prevent SIGSEGVs and the
> > mutator changed the value in the meantime.
> >
> > E.g.
> >
> >    oop o = load_from_heap(p);
> >    if (o == NULL) {
> >       return;
> >    }
> >    // do something with o; potential crash here when reloading from
> > p
> > instead of actually reusing o
> >
> > According to SAP particularly the IBM xlc compiler tends to to such
> > nasty things (which is completely okay from a language POV).
> >
> > The fix is to make the load from the heap a volatile load using the
> > new AccessBarrier API in cases this can happen.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8129440
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8129440/webrev/
>
> Looks good, Reviewed.
>

  thanks for your review.

Thomas
Reply | Threaded
Open this post in threaded view
|

Re: [10?] RFR (XS): 8129440: G1 crash during concurrent root region scan

Thomas Schatzl
In reply to this post by Erik Österlund-2
Hi Erik,

On Thu, 2017-12-07 at 09:33 +0100, Erik Österlund wrote:
> Hi Thomas,
>
> Looks good to me.
>
> Thanks,
> /Erik

  thanks for your review.

Thomas