RFR: 8237363 remove oop iterate verification

[Stefan Karlsson]

On Thu, 22 Oct 2020 07:40:03 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> There's verification code in the "oop iterate" framework that asserts that a pointer is "is in the heap". This works for most GCs, but ZGC *can* eagerly decommit the old relocation set pages, which means that pointers to the old / from copy of the object could point to memory that is currently not a part of the current heap.
>> 
>> To combat this in the past I've added a way for oop iterate closures to turn off this verification. However, every single time we add a new closure we have to consider if we can allow this verification check or if we have to remove it. Personally, I think this is a false abstraction and also widens the oop iterate closure interface. I previously proposed a patch that moved the verification code down into the oop iterate closures. It wasn't a huge patch, but I got push-back that it was convenient for other GCs to get this automatic verification, and the review stalled.
>> 
>> In this new patch I propose a different way to retain the verification. The realization is that most oop iterate closures have to deal with both compressed and non-compressed oops, so the code typically looks like this:
>> 
>> template <class T>
>> inline void G1ScanCardClosure::do_oop_work(T* p) {
>>   T o = RawAccess<>::oop_load(p);
>>   if (CompressedOops::is_null(o)) {
>>     return;
>>   }
>>   oop obj = CompressedOops::decode_not_null(o);
>> 
>> Therefore the suggest new place to put the is_in verification is in the CompressedOops::decode*. This injects the assert into almost all non-ZGC closures, and also to places that don't use the oop iterate closure framework. I think this is a neat workaround, and hope this patch is accepted this time.
>> 
>> I've tested this patch a few weeks ago, but will rerun the relevant tiers.
>
> src/hotspot/share/memory/filemap.cpp line 1740:
> 
>> 1738:   narrowOop n = CompressedOops::narrow_oop_cast(offset);
>> 1739:   if (with_current_oop_encoding_mode) {
>> 1740:     return cast_from_oop<address>(CompressedOops::decode_raw_not_null(n));
> 
> Why does this line skip verification now?

This code is used to setup the CDS archive. At that point in time, the heap isn't mapped yet, and there's no heap region at the suggested offset, so the new assert fails.
HeapRegion::is_in_reserved (this=0x0, p=0x7bfe00000)
HeapRegion::is_in (this=0x0, p=0x7bfe00000)
G1CollectedHeap::is_in (this=0x7ffff003a100, p=0x7bfe00000)
CompressedOops::decode_not_null (v=(unknown: -134479872))
FileMapInfo::decode_start_address (this=0x7ffff05d3e60, spc=0x7ffff05d4000, with_current_oop_encoding_mode=true)
FileMapInfo::start_address_as_decoded_with_current_oop_encoding_mode
FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode
FileMapInfo::map_heap_regions_impl
FileMapInfo::map_heap_regions
MetaspaceShared::map_archives
MetaspaceShared::initialize_runtime_shared_and_meta_spaces ()
Metaspace::global_initialize
universe_init ()
init_globals ()
Threads::create_vm

Note that the heap region is null (this=0x0). This also mean that the returned oop is not actually a valid oop at all, and this can sort of be seen by the immediate cast to address.

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

PR: https://git.openjdk.java.net/jdk/pull/797