RFR: 8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp

[David Holmes]

On Fri, 27 Oct 2023 15:40:58 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> Calls in instanceKlass.cpp and unsafe.cpp try to call an atomic load on method calls that could return nullptr. This patch ensures that nullptr is not passed into the load. Verified with tier1-5 tests.

Changes requested by dholmes (Reviewer).

src/hotspot/share/oops/instanceKlass.cpp line 2506:

> 2504:       // Use load_acquire due to competing with inserts
> 2505:       InstanceKlass* volatile* iklass = adr_implementor();
> 2506:       InstanceKlass* impl = (iklass != nullptr) ? Atomic::load_acquire(iklass) : nullptr;

This looks very klunky as we do a raw read, check it for null then re-read with acquire semantics. Cleaner IMO to do a raw read followed by a raw `OrderAccess::acquire()` and no need for a null check.

src/hotspot/share/prims/unsafe.cpp line 234:

> 232:   T get_volatile() {
> 233:     GuardUnsafeAccess guard(_thread);
> 234:     assert(addr() != nullptr, "Attempting to load from null pointer");

I don't see how `addr()` can be null unless `_obj` was null - which would be a usage error. So asserting `_obj != nullptr` in the constructor would seem better to me. I mean no point checking `addr()` here but not in other functions where we dereference it!

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

PR Review: https://git.openjdk.org/jdk/pull/16405#pullrequestreview-1703425012
PR Review Comment: https://git.openjdk.org/jdk/pull/16405#discussion_r1375776385
PR Review Comment: https://git.openjdk.org/jdk/pull/16405#discussion_r1375779895