RFR: 8340392: Handle OopStorage in location decoder [v4]

[Kim Barrett]

On Thu, 19 Sep 2024 05:42:18 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Another debugging QoL improvement.  Currently, when there is a pointer into `OopStorage` that we need to decode for the error log, we just print:
>> 
>>   0x00007ad45c169e10 into live malloced block starting at 0x00007ad45c169dd0, size 632, tag mtInternal
>> 
>> 
>> This is reported by NMT after [JDK-8304815](https://bugs.openjdk.org/browse/JDK-8304815). It is likely worse without NMT. We can actually decode which block in which `OopStorage` the address likely belongs to. This becomes handy when debugging GC crashes that involve `OopStorage`-handled roots.
>> 
>> This patch is able to print the following instead:
>> 
>> 
>>   0x0000000102c05bd0 is a pointer 2/64 into block 0 in oop storage "VM Global"
>
> Aleksey Shipilev has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Try to handle unaligned pointers well
>  - Indenting and formats

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/shared/oopStorageSet.cpp line 86:

> 84: 
> 85: bool OopStorageSet::print_containing(const void* addr, outputStream* st) {
> 86:   const void* aligned_addr = align_down(addr, alignof(oop*));

Should be `alignof(oop)`.

src/hotspot/share/gc/shared/oopStorageSet.cpp line 87:

> 85: bool OopStorageSet::print_containing(const void* addr, outputStream* st) {
> 86:   const void* aligned_addr = align_down(addr, alignof(oop*));
> 87:   if (aligned_addr != nullptr) {

I think the nullptr check should come first.  Pointer arithmetic can't produce a null pointer (because
of UB), so I think a sufficiently smart might be able to conclude that this test is always true in the
absence of UB in the alignment code.

src/hotspot/share/gc/shared/oopStorageSet.cpp line 88:

> 86:   const void* aligned_addr = align_down(addr, alignof(oop*));
> 87:   if (aligned_addr != nullptr) {
> 88:     for (uint i = 0; i < OopStorageSet::all_count; i++) {

Don't need the `OopStorageSet::` qualifier here, since we're in that class already.
But better would be to iterate over the storages using the provided iteration mechanism:

for (OopStorage* storage : Range<Id>()) {
   ...
}

test/hotspot/gtest/gc/shared/test_oopStorageSet.cpp line 139:

> 137:   void doit() {
> 138:     PrintContainingClosure cl;
> 139:     for (auto id: EnumRange<OopStorageSet::Id>()) {

Instead use

for (OopStorage* storage : OopStorageSet::Range<OopStorageSet::Id>()) {
   ...
}

test/hotspot/gtest/gc/shared/test_oopStorageSet.cpp line 166:

> 164:   {
> 165:     stringStream ss;
> 166:     bool printed = OopStorageSet::print_containing((char*)0x8, &ss);

Instead of hard-coded 0x8, instead use alignof(oop).

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

PR Review: https://git.openjdk.org/jdk/pull/21072#pullrequestreview-2314518336
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766234492
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766237573
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766241306
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766297311
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766282757