RFR: 8301222: Generalize check_release_entry in OopStorage

[Erik Österlund]

On Fri, 3 Feb 2023 15:23:35 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> When an oop handle is released to OopStorage, we want to check that its contents has been cleared appropriately, honouring the general contract with OopStorage. This is currently done with a raw access checking for null. However, the raw contents in memory might not be 0, just because the logical value is null. In particular, generational ZGC will have some low order colour bits set. This CR aims to address that.
>
> src/hotspot/share/gc/shared/oopStorage.cpp line 770:
> 
>> 768:   assert(entry != NULL, "Releasing NULL");
>> 769:   assert(!Universe::heap()->is_in(*reinterpret_cast<void* const*>(entry)),
>> 770:          "Releasing uncleared entry: " PTR_FORMAT, p2i(entry));
> 
> Why are the extra bits only in this particular case an issue? We compare to null in quite a few places, and there we probably just added code to strip these extra bits (or is already by previous code).
> That would be more expensive to do in the assert I guess, but much more clear to me what we are comparing to.

The reason the null check here is problematic, is that it's the only load check using a raw load. When you go through the Access API, you get an uncoloured null pointer as a result. But raw loads expose the internal encoding of the pointers, and null is not always encoded as 0 with generational ZGC. I'm not against stripping the colour bits here, but Kim didn't like that. Options on the table:

- Rewrite the code to use the Access API - problematic because a) it's not always safe to use barriers where this is called, and b) we don't want to give OopStorage knowledge of the reference strength
- Mask the bits - a bit ugly and makes assumptions about ZGC in the check, but gets the job done
- Use is_in instead of a null check to catch if the user forgot to clear - gets the job done, but makes some assumptions about how is_in is implemented.
- Define a CollectedHeap::contains_null(void*), possibly debug only - gets the job done, but clutters the top level GC API
- Disable the assert for ZGC - testing will show with other GCs if the user forgot to clear

I don't have any strong preferences. Do you have any opinions @tschatzl?

-------------

PR: https://git.openjdk.org/jdk/pull/12252