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

[Roman Kennke]

On Wed, 7 Aug 2024 18:50:47 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 incrementally with three additional commits since the last revision:
> 
>  - Style touchups
>  - Fixing ShenandoahReferenceProcessor
>  - Verifier fix

Mostly looks good, I have a few suggestions.

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

> 746: }
> 747: 
> 748: bool ShenandoahHeap::is_in_bounds(const void* p) const {

Is this the same as is_in_reserved()? Do we even need that new method?

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

> 757:     // objects during Full GC across the regions in not yet determinate state.
> 758:     return is_full_gc_move_in_progress() ||
> 759:            heap_region_containing(p)->is_active();

Should this also check against the region bounds?

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 96:

> 94: // Raw referent, it can be dead. You cannot dereference it, only use for nullptr
> 95: // and bitmap checks. The decoding uses a special-case inlined CompressedOops::decode
> 96: // method that bypasses normal oop-ness checks.

If you don't want to be treated like an actual oop, you could return a HeapWord* instead. That's still good enough for null- and bitmap-checking. Not sure if it causes a lot of casting around, if so then it's probably not worth it.

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

Marked as reviewed by rkennke (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20492#pullrequestreview-2236206495
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1715699307
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1715702322
PR Review Comment: https://git.openjdk.org/jdk/pull/20492#discussion_r1715705338