RFR: 8371216: oopDesc::print_value_on breaks if klass is garbage [v2]

[Thomas Stuefe]

On Fri, 7 Nov 2025 14:35:23 GMT, Paul Hübner <phubner at openjdk.org> wrote:

>> Hi all,
>> 
>> The `oopDesc::print_value_on` function checks if an oop is a string, and if so just prints the raw string. To do this, it needs to read the `klass()`. If the `klass()` reads garbage, one of many assertion errors is likely triggered.
>> 
>> For example, if G1's verification finds problematic oops, it will attempt to print them. If these oops have garbage (incorrect or racey) klasses, this will cause an assertion error, fail fast, and VM crash. G1 never finishes printing, which may make debugging more difficult. The developer can/will be made aware in other ways if the `klass()` is garbage, for example by being told that it is not in the metaspace.
>> 
>> We observed the above in Valhalla and already patched it there.
>> 
>> Testing: tiers 1-5 on Linux (x64, AArch64), macOS (x64, AArch64), Windows (x64).
>
> Paul Hübner has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Don't make a new function.

> > I would not add the helper function for only one case. We already have a bunch of helpers like these. Just test for oop != NULL at the call site.
> 
> In the scenario I am referring to, we had an always-non-null oop and went through `CompressedKlassPointers::decode_not_null` (which has like 12 assertions). Most of the time, we failed because of the klass range check. In rare cases for this particular instance, but quite often in often in other cases like [JDK-8366794](https://bugs.openjdk.org/browse/JDK-8366794), the klass was null. I don't think it's sufficient to just check for oop nullness.
> 
> I agree with you, it'd be nicer to check at the call site. How do you feel about inlining the proposed function into:
> 
> ```c++
>   if (obj != nullptr && obj->klass_without_asserts() == vmClasses::String_klass()) {
>     java_lang_String::print(obj, st);
>     print_address_on(st);
>   } else // [...]
> ```

Yes, that is what I meant.

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

PR Comment: https://git.openjdk.org/jdk/pull/28190#issuecomment-3509341804