RFR: 8231113: Adjust CollectedHeap::check_oop_location()

[Stefan Karlsson]

Hi Per,

I think both names are bad because the functions return false positives 
but the names give the impression that they won't. For example:

    // CMS forwards some non-heap value into the mark oop to reserve oops during
    // promotion, so we can't assert about obj alignment or that the forwardee is in heap
- virtual void check_oop_location(void* addr) const {}
+ virtual bool is_oop_location(void* addr) const { return true; }

It's obvious that this function returns true for pointers outside of the 
heap. Other implementations are not as bad, but they also return false 
positives. Users can't use is_oop_location to drive code logic, only to 
do sanity checks.

The check_oop_location functions didn't have that problem, but they 
couldn't be used in asserts. Did you consider changing the 
check_oop_location functions to return a bool instead of asserting?

    G1CollectedHeap* g1h = G1CollectedHeap::heap();
    // can't do because of races
    // assert(oopDesc::is_oop_or_null(obj), "expected an oop");
- g1h->check_oop_location(obj);
+ assert(g1h->check_oop_location(obj), "invalid oop location");

I think that would be less misleading to readers of the code.

Thanks,
StefanK

On 2019-09-17 15:02, Per Liden wrote:
> I propose we adjust and rename CollectedHeap::check_oop_location() 
> into CollectedHeap::is_oop_location() that returns a bool, to better 
> match its sibling CollectedHeap::is_oop() and how that is used.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8231113
> Webrev: http://cr.openjdk.java.net/~pliden/8231113/webrev.0
>
> /Per