RFR: 8349923: Refactor StackMapTable constructor and StackMapReader [v2]

[David Holmes]

On Fri, 14 Feb 2025 20:13:48 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> This patch refactors the `StackMapReader` to clean up the `StackMapTable` constructor. The `StackMapReader` class is extended to hold more fields used for verification purposes and allows for a simpler StackMapTable constructor. Additionally, in preparation for further changes to verification, the `_frame_count` may not be equal to the value reported by the classfile, so a new structure is used to generate the `_frame_array`. Verified with tier 1-5 tests
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Coleen comments

Looks good. A few minor comments/suggestions.

Thanks

src/hotspot/share/classfile/stackMapTable.cpp line 49:

> 47: }
> 48: 
> 49: void StackMapReader::check_offset(StackMapFrame* frame, TRAPS) {

TRAPS is unused

src/hotspot/share/classfile/stackMapTable.cpp line 54:

> 52:     _verifier->verify_error(
> 53:         ErrorContext::bad_stackmap(0, frame),
> 54:         "StackMapTable error: bad offset");

Suggestion:

    _verifier->verify_error(ErrorContext::bad_stackmap(0, frame),
                            "StackMapTable error: bad offset");

src/hotspot/share/classfile/stackMapTable.cpp line 60:

> 58: void StackMapReader::check_size(TRAPS) {
> 59:   if (_frame_count < _parsed_frame_count) {
> 60:     StackMapStream::stackmap_format_error("wrong attribute size", CHECK);

Suggestion:

    StackMapStream::stackmap_format_error("wrong attribute size", THREAD);

The method will return no matter what.

src/hotspot/share/classfile/stackMapTable.cpp line 67:

> 65:   assert(_stream->at_end(), "must be");
> 66:   if (_frame_count != _parsed_frame_count) {
> 67:     StackMapStream::stackmap_format_error("wrong attribute size", CHECK);

Suggestion:

    StackMapStream::stackmap_format_error("wrong attribute size", THREAD);

The method will return no matter what.

src/hotspot/share/classfile/stackMapTable.cpp line 110:

> 108:   }
> 109: 
> 110:   StackMapFrame *stackmap_frame = _frame_array->at(frame_index);

Suggestion:

  StackMapFrame* stackmap_frame = _frame_array->at(frame_index);

Pre-existing

src/hotspot/share/classfile/stackMapTable.hpp line 167:

> 165:   inline char* code_data()          const { return _code_data; }
> 166:   inline int32_t code_length()      const { return _code_length; }
> 167:   inline bool at_end() { return _stream->at_end(); }

Suggestion:

  inline int32_t get_frame_count()   const { return _frame_count; }
  inline StackMapFrame* prev_frame() const { return _prev_frame; }
  inline char* code_data()           const { return _code_data; }
  inline int32_t code_length()       const { return _code_length; }
  inline bool at_end()               const { return _stream->at_end(); }

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23645#pullrequestreview-2625014395
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960580404
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960582418
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960585457
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960585849
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960586368
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1960590243