On Thu, 7 May 2026 14:34:25 GMT, Joel Sikström <[email protected]> wrote:
> Hello, > > This PR fixes an issue in `JVM_IHashCode` where we don't handle an exception > occurring in the Java call to `ValueObjectMethods.valueObjectHashCode`. If an > exception is thrown, we still proceed to install the (bad) hashcode that is > based on a return value which is garbage data. > > To fix this, we should make sure to return if there is a pending exception. > > @xmas92 came up with a smart approach to encapsulate the behavior of wrapping > an exception in an internal error into a separate helper, which is > implemented in this PR as well. I've adapted other similar places where we > wrap exceptions like this as well. These use-cases are all Valhalla-specific, > so makes sense to change here IMO. > > Testing: > * Running Oracle's tier1-4 > * Test reproducer that fails before (installing a bad hashcode), and succeeds > with the changes in this PR (never installs a bad hashcode). > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Good catch on the missing return! The helper looks good. src/hotspot/share/utilities/exceptions.cpp line 478: > 476: } > 477: > 478: void Exceptions::wrap_exception_in_internal_error(const char* message, > JavaThread* THREAD) { Nit: we use `THREAD` when we need it for the macros e.g. `HAS_PENDING_EXCEPTION`. If the code doesn't use the macros then it could just be `JavaThread* current`. Though arguably we should be using TRAPS as this can throw. I realize `wrap_dynamic_exception` set the precedent for this formulation though, so not insisting on any changes. src/hotspot/share/utilities/exceptions.cpp line 479: > 477: > 478: void Exceptions::wrap_exception_in_internal_error(const char* message, > JavaThread* THREAD) { > 479: // If there is a pending exception and that is not an internal error, Suggestion: // If there is a pending exception that is not an Error, src/hotspot/share/utilities/exceptions.cpp line 480: > 478: void Exceptions::wrap_exception_in_internal_error(const char* message, > JavaThread* THREAD) { > 479: // If there is a pending exception and that is not an internal error, > 480: // clear it and wrap it in an internal error instead. Suggestion: // clear it and wrap it in an InternalError instead. ------------- Marked as reviewed by dholmes (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2410#pullrequestreview-4248834995 PR Review Comment: https://git.openjdk.org/valhalla/pull/2410#discussion_r3205740869 PR Review Comment: https://git.openjdk.org/valhalla/pull/2410#discussion_r3205716575 PR Review Comment: https://git.openjdk.org/valhalla/pull/2410#discussion_r3205742094
