RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

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

RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

Thomas Schatzl
Hi all,

  can I have reviews for this tiny change that fixes tripping an assert
when executing the post barrier for arrays for G1?

The issue is that when calling arraycopy to a zero sized array of j.l.O
a call to the post barrier is emitted. If that object at that point has
 been allocated so that its non-existent value array is located at the
end of the heap, the MemRegion passed to
G1SATBCardTableLoggingModRefBS::invalidate() has a start address just
beyond the heap the assert trips over (it has a zero-sized length).

Note that the G1 invalidation code is correct, i.e. handled the
situation correctly, just the assert is wrong in this case.

The suggested fix is to ignore zero-sized MemRegions like the other
card table implementation does. This will keep triggering the assert
for really wrong MemRegions (which is desired imho).

Thanks go to Alex who investigated and fixed a very similar issue with
the other collectors in JDK-8185591.

CR:
https://bugs.openjdk.java.net/browse/JDK-8182050
Webrev:
http://cr.openjdk.java.net/~tschatzl/8182050/webrev/
Testing:
Included jtreg test case, checking that the issue does not occur with
the change anymore.

Thanks,
  Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

Erik Helin-2
On 11/22/2017 02:18 PM, Thomas Schatzl wrote:

> Hi all,
>
>    can I have reviews for this tiny change that fixes tripping an assert
> when executing the post barrier for arrays for G1?
>
> The issue is that when calling arraycopy to a zero sized array of j.l.O
> a call to the post barrier is emitted. If that object at that point has
>   been allocated so that its non-existent value array is located at the
> end of the heap, the MemRegion passed to
> G1SATBCardTableLoggingModRefBS::invalidate() has a start address just
> beyond the heap the assert trips over (it has a zero-sized length).
>
> Note that the G1 invalidation code is correct, i.e. handled the
> situation correctly, just the assert is wrong in this case.
>
> The suggested fix is to ignore zero-sized MemRegions like the other
> card table implementation does. This will keep triggering the assert
> for really wrong MemRegions (which is desired imho).
>
> Thanks go to Alex who investigated and fixed a very similar issue with
> the other collectors in JDK-8185591.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8182050
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8182050/webrev/

Looks good, Reviewed. Thanks Thomas for taking care of this (and Alex
for finding the cause for JDK-8185591).

Erik

> Testing:
> Included jtreg test case, checking that the issue does not occur with
> the change anymore.
>
> Thanks,
>    Thomas
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

Thomas Schatzl
Hi Erik,

On Wed, 2017-11-22 at 16:14 +0100, Erik Helin wrote:

> On 11/22/2017 02:18 PM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for this tiny change that fixes tripping an
> > assert when executing the post barrier for arrays for G1?
> > [...]
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8182050
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8182050/webrev/
>
> Looks good, Reviewed. Thanks Thomas for taking care of this (and
> Alex for finding the cause for JDK-8185591).

Thanks for your review,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

Stefan Johansson
In reply to this post by Erik Helin-2


On 2017-11-22 16:14, Erik Helin wrote:

> On 11/22/2017 02:18 PM, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for this tiny change that fixes tripping an assert
>> when executing the post barrier for arrays for G1?
>>
>> The issue is that when calling arraycopy to a zero sized array of j.l.O
>> a call to the post barrier is emitted. If that object at that point has
>>   been allocated so that its non-existent value array is located at the
>> end of the heap, the MemRegion passed to
>> G1SATBCardTableLoggingModRefBS::invalidate() has a start address just
>> beyond the heap the assert trips over (it has a zero-sized length).
>>
>> Note that the G1 invalidation code is correct, i.e. handled the
>> situation correctly, just the assert is wrong in this case.
>>
>> The suggested fix is to ignore zero-sized MemRegions like the other
>> card table implementation does. This will keep triggering the assert
>> for really wrong MemRegions (which is desired imho).
>>
>> Thanks go to Alex who investigated and fixed a very similar issue with
>> the other collectors in JDK-8185591.
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8182050
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8182050/webrev/
>
> Looks good, Reviewed. Thanks Thomas for taking care of this (and Alex
> for finding the cause for JDK-8185591).
I agree, great work guys! Change looks good,

Stefan

>
> Erik
>
>> Testing:
>> Included jtreg test case, checking that the issue does not occur with
>> the change anymore.
>>
>> Thanks,
>>    Thomas
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR (XS): 8182050: assert(_whole_heap.contains(p)) failed: Attempt to access p out of bounds of card marking array's _whole_heap

Thomas Schatzl
Hi,

On Wed, 2017-11-22 at 16:44 +0100, Stefan Johansson wrote:
>
> On 2017-11-22 16:14, Erik Helin wrote:
> > On 11/22/2017 02:18 PM, Thomas Schatzl wrote:
> > > Hi all,
> > >
> > >    can I have reviews for this tiny change that fixes tripping an
> > > assert when executing the post barrier for arrays for G1?
> > >
[...]

> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8182050
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8182050/webrev/
> >
> > Looks good, Reviewed. Thanks Thomas for taking care of this (and
> > Alex
> > for finding the cause for JDK-8185591).
>
> I agree, great work guys! Change looks good,

Thanks for your review,
  Thomas