On Wed, 25 Oct 2023 10:01:29 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:
>> Not sure what you're saying here. As far as I understand the intent of this >> code is to check whether the handle is of a certain type, and if it's not >> recognized, return `JNIInvalidRefType`. So, I'm not sure there should be any >> `ShouldNotReachHere()` in this code. >> >> We can add an `else` branch that returns `JNIInvalidRefType` though. > > The old code would throw `ShouldNotReachHere()` if we did not recognize the > handle type: > https://github.com/openjdk/jdk/blob/d2d1592dd94e897fae6fc4098e43b4fffb6d6750/src/hotspot/share/runtime/jniHandles.cpp#L207 > > I think the new code should still keep it, like so: > > > } else if (is_local_handle(thread, handle) || is_frame_handle(thread, > handle)) { > ... > } else { > ShouldNotReachHere(); > } > > > That way, if we ever have another handle type, we would hit the `else` branch > instead of silently returning `JNIInvalidRefType`. That is, our condition > chain would still be exhaustive, catching unexpected values explicitly. But that just re-introduces the bug? I don't see how we can have both `ShouldNotReachHere()` in the 'else branch' and return `JNIInvalidRefType` for unrecognized handles at the same time. Not that this function is used to check potentially stale/invalid/corrupt JNI handles, so the input handle can be complete garbage. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16349#discussion_r1371485862
