RFR: 8237363: Remove automatic is in heap verification in OopIterateClosure

[Thomas Schatzl]

Hi,

On 17.01.20 14:31, Stefan Karlsson wrote:
> Hi all,
> 
> Please review this patch to remove the automatic "is in heap" 
> verification from OopIterateClosure.
> 
> https://cr.openjdk.java.net/~stefank/8237363/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8237363
> 
> OopIterateClosure provides some automatic verification that loaded 
> objects are inside the heap. Closures can opt out from this by 
> overriding should_verify_oops().
> 
> I propose that we move this verification, and the way to turn it off, 
> and instead let the implementations of the closures decide the kind of 
> verification that is appropriate. I want to do this to de-clutter the 
> closure APIs a bit.
> 

While the change is correct, I am not really convinced it is a good idea 
to trade verification in one place to the same verification in many place.

The closure API does not seem to be particularly "cluttered up" by this 
particular API to me. It is true that other code typically has many 
other asserts that would fail anyway, but it would be an additional 
safety net when writing new closures.

This is not a hard no for this change, but is there something else you 
are planning to do in this area where this code would be in the way?

> I've gone through all OopIterateClosures that don't override 
> should_verify_oops() and added calls to 
> assert_oop_field_points_to_object_in_heap[_or_null] where the closures 
> didn't have equivalent checks.
> 
> A lot of the places didn't explicitly check that the object is within 
> the heap but they would check for other things like:
> - Is the corresponding bit index within the range
> - Is the heap region index within range
> - Is the object in the reserved heap range (weaker than is_in)
> 
> I've added asserts to those places. If you think I should remove some of 
> them, please let me now.
> 
> Tested with tier1-3
> 

Thanks,
   Thomas