RFR: 8358735: GenShen: block_start() may be incorrect after class unloading [v12]

[Y. Srinivas Ramakrishna]

On Tue, 11 Nov 2025 22:42:46 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add two comments
>
> src/hotspot/share/gc/shenandoah/shenandoahMarkBitMap.cpp line 70:
> 
>> 68:   assert(limit >= r->bottom(), "limit must be more than bottom");
>> 69:   assert(addr <= tams, "addr must be less than TAMS");
>> 70: #endif
> 
> Wouldn't it make more sense for these checks to move to the caller? It appears to me to be a leakage of abstraction to test these conditions here. We should be able to return the address for the marked bit found without interpreting the semantics of the bits themselves?

I notice that this is the case for the `get_next_...` version below as well; if my comment makes some sense, this can be addressed separately.

Perhaps frugality in testing the conditions required us to site these assertions here, which I kind of understand, although the right thing in that case is to have the wrapper class, viz. `ShenandoahMarkingContext` make those checks before calling here.

> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 252:
> 
>> 250: 
>> 251:   HeapWord* right = MIN2(region->top(), end_range_of_interest);
>> 252:   HeapWord* end_of_search_next = MIN2(right, tams);
> 
> Does the caller ensure that `tams` is always valid (e.g. when `ctx == nullptr`)?

The caller seems to allow for `tams==nullptr` and `ctx==nullptr`. In that case wouldn't we get `end_of_search_next==nullptr`?

> src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp line 181:
> 
>> 179:       // common.
>> 180:       assert(ctx != nullptr || heap->old_generation()->is_parsable(), "Error");
>> 181:       HeapWord* p = _scc->first_object_start(dirty_l, ctx, tams, right);
> 
> TODO: Wondering if we need to pass both tams and right, or just the max of the two. Will look at `first_object_start()`.

Ah, looks like at this point we might potentially have `ctx == nullptr` and `tams == nullptr`. I wonder if we can do better here in terms of passing a sensible single `right` and dispense with passing `tams` entirely? Let me go back and look at the implementation of the method again.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2516079967
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2516338478
PR Review Comment: https://git.openjdk.org/jdk/pull/27353#discussion_r2516323774