RFR: 8318462: [GenShen] Prevent unsafe access to displaced mark-word [v3]

[Y. Srinivas Ramakrishna]

On Tue, 31 Oct 2023 23:21:43 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> The add method of ShenandoahAgeCensus explicitly recognizes and uses the sentinel value, and explicitly skips sentinels that are used to signal that the age could not be read:
>> 
>> 
>> CENSUS_NOISE(void ShenandoahAgeCensus::add(uint obj_age, uint region_age, uint region_youth, size_t size, uint worker_id) {)
>> NO_CENSUS_NOISE(void ShenandoahAgeCensus::add(uint obj_age, uint region_age, size_t size, uint worker_id) {)
>>   if (obj_age <= markWord::max_age) {
>>     assert(obj_age < MAX_COHORTS && region_age < MAX_COHORTS, "Should have been tenured");
>> #ifdef SHENANDOAH_CENSUS_NOISE
>>     // Region ageing is stochastic and non-monotonic; this vitiates mortality
>>     // demographics in ways that might defeat our algorithms. Marking may be a
>>     // time when we might be able to correct this, but we currently do not do
>>     // this. Like skipped statistics further below, we want to track the
>>     // impact of this noise to see if this may be worthwhile. JDK-<TBD>.
>>     uint age = obj_age;
>>     if (region_age > 0) {
>>       add_aged(size, worker_id);   // this tracking is coarse for now
>>       age += region_age;
>>       if (age >= MAX_COHORTS) {
>>         age = (uint)(MAX_COHORTS - 1);  // clamp
>>         add_clamped(size, worker_id);
>>       }
>>     }
>>     if (region_youth > 0) {   // track object volume with retrograde age
>>       add_young(size, worker_id);
>>     }
>> #else   // SHENANDOAH_CENSUS_NOISE
>>     uint age = MIN2(obj_age + region_age, (uint)(MAX_COHORTS - 1));  // clamp
>> #endif  // SHENANDOAH_CENSUS_NOISE
>>     get_local_age_table(worker_id)->add(age, size);
>>   } else {
>>     // update skipped statistics
>>     CENSUS_NOISE(add_skipped(size, worker_id);)
>>   }
>> }
>> 
>> 
>> I think we should leave in place the original code, where all of the special handling of the sentinel values is relegated to the ShenandoahAgeCensus's add method, with no special handling needed at the caller.
>
> Thanks.  That looks good.  I concur.

I realize though that your comment kind of applies to the evacuation tracker code that needs to be cognizant of the change in the semantics of `get_object_age` following Roman's change.

This is the relevant call at:
https://github.com/openjdk/shenandoah/blob/113a3d2bbec5679a9c25550fc825feb75a7b41e8/src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp#L491
which lands us at:
https://github.com/openjdk/shenandoah/blob/113a3d2bbec5679a9c25550fc825feb75a7b41e8/src/hotspot/share/gc/shenandoah/shenandoahEvacTracker.cpp#L60
and thence at:
https://github.com/openjdk/shenandoah/blob/113a3d2bbec5679a9c25550fc825feb75a7b41e8/src/hotspot/share/gc/shared/ageTable.hpp#L63
which might either assert or silently overflow past the end of the table.

It would be useful to filter those records out in ShenandoahEvacuationStats::record_age() or at the caller.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/343#discussion_r1378299556