RFR: 8301222: Generalize check_release_entry in OopStorage

[Erik Österlund]

On Fri, 3 Mar 2023 01:00:23 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

Thanks for the feedback. I updated the PR with the contains_null approach. I like it too.

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

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