RFR: 8231113: Adjust CollectedHeap::check_oop_location()

[Per Liden]

Thanks for reviewing!

/Per

On 9/19/19 2:35 PM, Erik Österlund wrote:
> Hi Per,
> 
> With this approach we are essentially removing the support for catching 
> forwarding to something not in the heap. I think the original intention 
> of the assert might have been to catch scenarios where an adjust phase 
> visits the same oop* twice. That would cause the second adjustment to 
> read a random value in the mark word, likely not in the heap, and 
> trigger the assert. Having said that, I think that such bugs will be 
> easily found anyway as it will very likely crash or fail verification 
> later on. So I think we will still catch any such bugs. And I can't 
> recall this assertion ever helping. So I think I am okay with this 
> relaxation.
> 
> Thanks,
> /Erik
> 
> On 2019-09-19 13:23, Per Liden wrote:
>> 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
>>>
>