RFR: 8292912: Make guard card in CardTable inaccessible [v2]

[Albert Mingkun Yang]

On Mon, 5 Sep 2022 12:45:10 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   remove
>
> src/hotspot/share/gc/g1/g1CardTable.cpp line 56:
> 
>> 54: 
>> 55:   size_t num_cards = cards_required(_whole_heap.word_size());
>> 56:   _last_valid_index = num_cards - 1;
> 
> It looks like `_last_valid_index` can be removed: it's only use is in asserts or very infrequently used methods afaict which imho does not warrant an explicit member now.

I agree, but this should probably be dealt with in another PR -- this change doesn't affect the value of `_last_valid_index`.

> src/hotspot/share/gc/shared/cardTable.cpp line 97:
> 
>> 95:   // each card takes 1 byte; + 1 for the guard card
>> 96:   size_t num_bytes = num_cards + 1;
>> 97:   _byte_map_size = compute_byte_map_size(num_bytes);
> 
> This seems a pre-existing bug/oddity, but `_byte_map_size` includes the guard card; there are several asserts (in `CardTable::byte_for` and `PSCardTable::is_valid_card_address`) that check:
> 
> assert(result >= _byte_map && result < _byte_map + _byte_map_size,
> 
> which seems weird to me as the card at the guard card is somehow assumed to be a valid part of the card table.
> I believe the `_byte_map_size` should not include the guard card itself (which we replaced by a guard page where access to it will cause an immediate error anyway), but pass the `+1` to the `ReservedSpace` constructor directly (and maybe adjust other locations that use it to print stuff, i.e. introduce a helper method that adds that +1?)
> 
> What do you think?

That assert is indeed too weak. Maybe the longer-term plan can be to drop guard card/page completely, like what's in G1. Having "random" `+1` in the code could be confusing.

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

PR: https://git.openjdk.org/jdk/pull/10020