RFR: 8278874: tighten VerifyStack constraints [v2]

[Manuel Hässig]

On Fri, 4 Jul 2025 02:33:33 GMT, Dean Long <dlong at openjdk.org> wrote:

>> The VerifyStack logic in Deoptimization::unpack_frames() attempts to check the expression stack size of the interpreter frame against what GenerateOopMap computes.  To do this, it needs to know if the state at the current bci represents the "before" state, meaning the bytecode will be reexecuted, or the "after" state, meaning we will advance to the next bytecode.  The old code didn't know how to determine exactly what state we were in, so it checked both.  This PR cleans that up, so we only have to compute the oopmap once.  It also removes old SPARC support.
>
> Dean Long has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix optimized build

Thank you for this enhancement, @dean-long! I made a first pass to try and understand the logic, but ended up only commenting on cosmetics. I'll do a second pass next week.

src/hotspot/share/runtime/deoptimization.cpp line 847:

> 845: 
> 846: #ifndef PRODUCT
> 847: #ifdef ASSERT

Why is both `NOT_PRODUCT` and `ASSERT` needed here? So far, I thought that `ASSERT` implies `NOT_PRODUCT`.

src/hotspot/share/runtime/deoptimization.cpp line 939:

> 937:     bool is_top_frame = true;
> 938:     int callee_size_of_parameters = 0;
> 939:     for (int i = 0; i < cur_array->frames(); i++) {

I would suggest renaming `i`to `frame_idx` because there is one usage 50 lines down that would be much more clear with a more verbose name.

src/hotspot/share/runtime/deoptimization.cpp line 947:

> 945: 
> 946:       // Get the oop map for this bci
> 947:       InterpreterOopMap mask;

Perhaps you could move that down to line 977. It would just be one less variable to keep track.

src/hotspot/share/runtime/deoptimization.cpp line 950:

> 948:       int cur_invoke_parameter_size = 0;
> 949:       int top_frame_expression_stack_adjustment = 0;
> 950:       int bci = iframe->interpreter_frame_bci();

`bci` is only used in the `BytecodeStream` constructor below. I would suggest to just call `iframe->interpreter_frame_bci()` in the constructor and forego the variable definition.

src/hotspot/share/runtime/deoptimization.cpp line 999:

> 997:             (iframe_expr_size == mask.expression_stack_size() + callee_size_of_parameters)
> 998:             ))))
> 999:       {

Suggestion:

      int iframe_expr_size = iframe->interpreter_frame_expression_stack_size();
      int expr_stack_size_before = iframe_expr_size + (is_top_frame ? top_frame_expression_stack_adjustment : 0);

      if (!((is_top_frame && exec_mode == Unpack_exception && iframe_expr_size == 0) ||
            (reexecute ?
             (expr_stack_size_before == mask.expression_stack_size() + cur_invoke_parameter_size) :
             (iframe_expr_size == mask.expression_stack_size() + callee_size_of_parameters)
            ))) {

These parentheses can be simplified a bit.

src/hotspot/share/runtime/vframeArray.cpp line 195:

> 193:     Bytecodes::Code code = Bytecodes::code_at(method(), bcp);
> 194:     assert(!Interpreter::bytecode_should_reexecute(code), "should_reexecute mismatch");
> 195:   }

This might be a candidate for `#ifdef ASSERT`?
Suggestion:

#ifdef ASSERT
  if (!reexec) {
    address bcp = method()->bcp_from(bci());
    Bytecodes::Code code = Bytecodes::code_at(method(), bcp);
    assert(!Interpreter::bytecode_should_reexecute(code), "should_reexecute mismatch");
  }
#endif

src/hotspot/share/runtime/vframeArray.cpp line 239:

> 237:       pc = Interpreter::deopt_reexecute_entry(method(), bcp);
> 238:     }
> 239:     assert(reexecute, "must be");

This assert is a bit redundant with the condition on this branch and `reexecute` not being assigned to.
Suggestion:

src/hotspot/share/runtime/vframeArray.hpp line 1:

> 1: /*

Please update the copyright year in this file.

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/26121#pullrequestreview-3050547887
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227775314
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227853547
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227821569
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227832230
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227790942
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227879608
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227895145
PR Review Comment: https://git.openjdk.org/jdk/pull/26121#discussion_r2227887220