RFR: 8337981: ShenandoahHeap::is_in should check for alive regions [v3]

[Roman Kennke]

On Wed, 14 Aug 2024 17:58:11 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> The expected behavior of `CollectedHeap::is_in` is to check whether the object belongs to the committed parts of the heap:
>> https://github.com/openjdk/jdk/blob/d19ba81ce12a99de1114c1bfe67392f5aee2104e/src/hotspot/share/gc/shared/collectedHeap.hpp#L273-L276
>> 
>> This is useful to check if object resides in the parts of the heap the GC knows are not dead. Yet, Shenandoah's check just verifies that oop is within the heap bounds. So `is_in` check for an object that is in trashed/empty region would pass by accident, and we will miss detecting bugs. This should be rectified. I believe "committed" is too weak for the test as well, since we really want to know if we can touch the object, i.e. if it is in active region.
>> 
>> I re-wired assertions/verification code to be clear whether we check for heap bounds or actual in-heap conditions.
>> 
>> Deeper testing revealed that reference processing code potentially loads a dead referent, but only to null-check it, or ask bitmap about it. Still, more precise `in_heap` check fails asserts in `CompressedOops::decode`. That required a bit of touchup as well.
>> 
>> Additional testing:
>>  - [x] Linux AArch64 server fastdebug, `all` with `-XX:+UseShenandoahGC`
>>  - [x] Linux AArch64 server fastdebug, `all` with `-XX:+UseShenandoahGC -XX:+ShenandoahVerify`
>
> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8337981-shenandoah-is-in
>  - Even stronger is_in check
>  - Use is_in_reserved
>  - Drop raw_referent to HeapWord*
>  - Add some overrides
>  - Merge branch 'master' into JDK-8337981-shenandoah-is-in
>  - Style touchups
>  - Fixing ShenandoahReferenceProcessor
>  - Verifier fix
>  - Fix

Looks good now. You might want to improve the asserts similar to is_in() (up to you).

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp line 214:

> 212: 
> 213:   ShenandoahHeapRegion* obj_reg = heap->heap_region_containing(obj);
> 214:   if (!heap->is_full_gc_move_in_progress() && !obj_reg->is_active()) {

This is essentially ShenandoahHeap::is_in(), right? Might also want to check for obj < region->top() here? Or use is_in() to begin with?

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp line 249:

> 247:     // Step 3. Check that forwardee points to correct region, unless we are in Full GC.
> 248:     if (!heap->is_full_gc_move_in_progress()) {
> 249:       if (!fwd_reg->is_active()) {

Same here?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 740:

> 738:     // Now check if we point to a live section in active region.
> 739:     ShenandoahHeapRegion* r = heap_region_containing(p);
> 740:     return (r->is_active() && p < r->top());

I'm kinda surprised that there is no ShHeapRegion::is_in() (or variants), but ok. *shrugs*

-------------

Marked as reviewed by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20492#pullrequestreview-2238921696
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1717387502
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1717388712
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1717391321