RFR: JDK-8222545: Safe klass asserts

[Roman Kennke]

>>>>>> To add a little more detail, I could move the change up into 
>>>>>> is_objArray(), but I don't want to expose it to any non-assert 
>>>>>> paths. Therefore I could do 2 different impls there, guarded by 
>>>>>> #ifdef ASSERT but I don't think it's a good idea to behave 
>>>>>> differently under ASSERT, that kindof defeats the point of assert, 
>>>>>> right?
>>>>>>
>>>>>> What do you think ?
>>>>>
>>>>> I don't follow your argument. Under asserts you need to access the 
>>>>> klass pointer "safely" but otherwise you do not. So there are two 
>>>>> behaviours related to accessing the klass pointer anyway. I'd 
>>>>> rather see that encapsulated in the accessor.
>>>>>
>>>>> I assume it's not just asserts but any debug only code that wants 
>>>>> to access the klass pointer.
>>>>
>>>> In general, for any runtime calls into oopDesc::klass() the access 
>>>> should be safe. The acrobatics is only necessary for *GC-internal* 
>>>
>>> This is the part I don't quite understand, and goes back to my 
>>> initial question. Why are you doing these operations on from-space 
>>> objects? I'm thinking you should be in a position in the GC to make 
>>> sure this can never happen. If you need to do that in the GC (which 
>>> is fine), then the GC could apply a "resolve" function to get the 
>>> to-space object, and call size() (or whatever) on that object. This 
>>> shouldn't have to leak out of the GC, right?
>>
>> It is a problem when we are about to evacuate an object. Then we need 
>> to know its size in order to allocate and copy an appropriate chunk. 
>> The problem is that this part is racy: two threads (e.g. two Java 
>> threads via barrier, or one Java thread vs one GC thread) might 
>> compete over this: both would create a copy of the object, but 
>> ultimately only one would succeed (by CASing the fwd pointer). 
>> Therefore, getting hold of the object size is racy, by design, and 
>> this requires to resolve the _klass. Now, we can do that ahead of 
>> time, and call oopDesc::size_given_klass() and all would be good, 
>> except that size_given_klass() asserts that the object is indeed of 
>> the given klass, and hence fetches _klass again, which, at this point, 
>> is racy. Solving this inside the GC would require to basically copy 
>> all the machinery to get hold of object size into the GC. Are you 
>> asking me to do that?
>>
>> About the oop_oop_iterate() paths, I will revisit them. We should 
>> never actually need to iterate over a from-space object.
> 
> Ok, so the only thing you really want to do it not assert in
> ObjArrayKlass::oop_size() and TypeArrayKlass::oop_size(), correct? Given 
> that oopDesc::size_given_klass() exists to solve these kind of 
> concurrency issues, I would have understood if your patch just removed 
> the offending asserts, with the argument that you can't make these kind 
> of asserts/assumptions there.
> 
> In my view, that would be much easier to swallow, compared to adding 
> clue to CollectedHeap to try to workaround the asserts.

Ha! That was my first reaction when I've hit the asserts: why the heck 
should I assert something about the Klass* when I am in that Klass? But 
I can see that it serves a purpose: it ensures that size_given_klass() 
is not accidentally called on an object with a different Klass* and I 
wanted to preserve that guarantee. If you're fine with dropping this 
instead, then I'll do that.

What do you think about the assert in oopDesc::klass() ? It exists to 
ensure we're not accidentally doing an unsafe access on klass(). If 
called the wrong way without the assert, it would sure enough blow up, 
but might be harder to track down. Can we leave that assert plus 
corresponding interface in CollectedHeap there?

Roman