RFR: 8301222: Generalize check_release_entry in OopStorage

[Kim Barrett]

On Mon, 6 Feb 2023 08:44:18 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

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

`CollectedHeap::contains_null(const oop*)` seems like the pedantically correct
approach. (I think the argument shouldn't be void*, by the way.) Disabling the
check for ZGC seems like the simplest, and I agree that testing with other GCs
should be sufficient to catch problems. Might also need to disable for
Shenandoah? Regardless of the approach taken, some commentary is needed to
explain what's going on, either with the new `contains_null` function or here
if that approach isn't taken.

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

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