On Tue, 10 Sep 2024 07:53:23 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> src/hotspot/share/gc/shared/collectedHeap.cpp line 232: >> >>> 230: } >>> 231: >>> 232: // With compact headers, we can't safely access the class, due >> >> Suggestion: >> >> // With compact headers, we can't safely access the klass, due >> >> >> This is the case why? Because we might not have copied the header yet? Is >> this method actually ever used while the forwarded object is unstable? >> Given this is used for verification only afaik, we should make an effort to >> provide that check. > > With compact headers, we can't safely access the Klass* when the object has > been forwarded, because non-full-GC-forwarding temporarily overwrites the > mark-word, and thus the Klass*, with the forwarding pointer, and here we have > no way to make a distinction between Full-GC and regular GC forwarding. > > I improved the code to make the check when the object is not forwarded. Not > sure if we could/should do more (e.g. pass around is_full argument to make > the distinction, or find the - possibly few - places where we might call > is_oop() on from-space objects in regular GC and do the check in a > forwardee-safe way?). Ah, I found it! It seems only the ShenandoahVerifier calls oop_iterate() on from_space objects, which can have a forwarding, which would mess with the object's Klass*. We're lucky because that iterator doesn't visit the Klass*. I see the following ways out: - The caller must ensure that the oop is ok and Klass* is accessible. I could do that in the ShenandoahVerifier. It kinda defeats the point, though, we want the verifier operate on the 'raw' object, not necessarily the forwardee. - Next easy way out would be to use 'this' instead of obj->klass(). Should makes sense, because it should always be the same. Using 'this' in the assert (this->is_array_klass()) is kinda bogus, though. And asserting (this == obj->klass()) would be nice, but would have the same problem as before where we would need to exclude UCOH for the case where Shenandoah needs it. In-fact, this is done already in oopDesc::oop_iterate_backwards(), but also excluding UCOH. - We could add a hook in the iterator that gives the Klass* for a given oop, which can then be overridden by the actual iterator to do the right thing, e.g. load the Klass* from the forwardee. WDYT? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1751770293
