RFR: 8352185: Shenandoah: Invalid logic for remembered set verification [v13]

[Y. Srinivas Ramakrishna]

On Thu, 20 Mar 2025 22:48:24 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> There are some scenarios in which GenShen may have improper remembered set verification logic:
>> 
>> 1. Concurrent young cycles following a Full GC:
>> 
>> In the end of ShenandoahFullGC, it resets bitmaps for the entire heap w/o resetting marking context to be incomplete, but ShenandoahVerifier has code like below to get a complete old marking context for remembered set verification
>> 
>> 
>> ShenandoahVerifier  
>> ShenandoahMarkingContext* ShenandoahVerifier::get_marking_context_for_old() {
>>   shenandoah_assert_generations_reconciled();
>>   if (_heap->old_generation()->is_mark_complete() || _heap->gc_generation()->is_global()) {
>>     return _heap->complete_marking_context();
>>   }
>>   return nullptr;
>> }
>> 
>> 
>> For the concurrent young GC cycles after a full GC, the old marking context used for remembered set verification is stale, and may cause unexpected result. 
>> 
>> 2. For the impl of `ShenandoahVerifier::get_marking_context_for_old` mentioned above, it always return a marking context for global GC, but marking bitmaps is already reset before before init-mark, `ShenandoahVerifier::help_verify_region_rem_set` always skip verification in this case. 
>> 
>> 3. ShenandoahConcurrentGC always clean remembered set read table, but only swap read/write table when gc generation is young, this issue causes remembered set verification before init-mark to use a completely clean remembered set, but it is covered by issue 2. 
>> 
>> 4. After concurrent young cycle evacuates objects from a young region, it update refs using marking bitmaps from marking context, therefore it won't update references of dead old objects(is_marked(obj) is false: obj is not marking strong/weak and it is below tams). In this case, if the next cycle if global concurrent GC, remembered set can't be verified before init-mark because of the dead pointers. 
>> 
>> ### Solution
>> * After a full GC, always set marking completeness flag to false after reseting the marking bitmaps.
>> * Because there could be dead pointers in old gen were not updated to point to new address after evacuation and refs update, we should disable rem-set validation before init-mark&update-refs if old marking context is incomplete. 
>> 
>> ### Test
>> - [x] `make test TEST=hotspot_gc_shenandoah`
>> - [x] GHA
>
> Xiaolong Peng has updated the pull request incrementally with one additional commit since the last revision:
> 
>   tide up

I think the change can be pushed as is, but I am not convinced that the verification can't be tightened when old marking information is missing as long as we have a valid TAMS and there are no unparsable objects (which should only happen when coalease-&-fill has been interrupted, leaving dead objects with x-gen pointers that would cause false positives or upon class unloading when dead objects may end up being unparsable). The current condition of skipping verification when old bit maps are cleared seems to miss verification opportunities that would be valid after a completed C&F.

Left some related comments, but I won't hold back this PR further. The tightening can be done subsequently (and I am happy to pick that up afterwards as needed).

Thanks for your patience with my tardy and long-winded reviews! :-)

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 1060:

> 1058:   VerifyRememberedSet verify_remembered_set = _verify_remembered_before_marking;
> 1059:   if (_heap->mode()->is_generational() &&
> 1060:       !_heap->old_generation()->is_mark_complete()) {

Why not the following stronger condition to skip verification? My sense is that the only case we cannot verify is if we do not have marking info _and_ old gen has been left "unparsable" (because of an incomplete/interrupted C&F which may have us look at dead objects -- that are either unparsable because of class unloading, or are parsable but hold cross-gen pointers). In all other cases, we can do a safe and complete verification.


   is_generational() && !old_gen->is_mark_complete() && !old_gen->is_parsable()

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 1125:

> 1123:   VerifyRememberedSet verify_remembered_set = _verify_remembered_before_updating_references;
> 1124:   if (_heap->mode()->is_generational() &&
> 1125:       !_heap->old_generation()->is_mark_complete()) {

Same comment re stronger condition as previous one above.

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

Marked as reviewed by ysr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24092#pullrequestreview-2715476210
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2013133891
PR Review Comment: https://git.openjdk.org/jdk/pull/24092#discussion_r2013142910