RFR: 8231113: Adjust CollectedHeap::check_oop_location()

[Erik Österlund]

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