RFR: 8231113: Adjust CollectedHeap::check_oop_location()

[Per Liden]

Hi Stefan,

On 2019-09-17 21:48, Stefan Karlsson wrote:
> 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.

I see your point. After looking more at how we use check_oop_location() 
today, I'm more and more thinking that it's just the wrong abstraction. 
In CMS we're "checking oops" that we know aren't oops, so we're forced 
to disable check_oop_location() completely. The checks done for filler 
objects is sort of strange too. We're filling a space outside of the 
allocated part of a TLAB/region, so it becomes a philosophical 
discussion, where this is no "correct" answer, whether that is a valid 
oop location or not.

I actually think I'd prefer removing check_oop_location() all together, 
with the rational that some of the asserts where it's used today are 
"conceptually wrong". In the other cases (e.g. in CompressedOops and 
G1), we can make valid checks using better mechanisms.

So, how about this:

http://cr.openjdk.java.net/~pliden/8231113/webrev.1

Passed tier1-3.

cheers,
Per

> 
> 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
>