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

[Aleksey Shipilev]

On Thu, 19 Sep 2024 06:31:49 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

Right, dang. I missed that we carry `oop`, not `oop*` array in `Block`. Fixed.

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

OK, some day I will internalize all these landmines :)

> 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>()) {
>    ...
> }

Fixed.

> 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>()) {
>    ...
> }

Fixed.

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

Right. Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766382422
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766381514
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766380800
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766380908
PR Review Comment: https://git.openjdk.org/jdk/pull/21072#discussion_r1766381089