RFR: 8349923: Refactor StackMapTable constructor and StackMapReader

[Coleen Phillimore]

On Fri, 14 Feb 2025 18:28:51 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. Verified with tier 1-5 tests

This refactoring makes a lot of sense but I haven't fully reviewed and understand how the number of frames can be different.  But these are some initial comments from the first reading.

Never mind, I do know why frame_count might change with your upcoming changes.

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

> 35:   if (_frame_count > 0) {
> 36:     GrowableArray<StackMapFrame*>* tmp_frame_array = new GrowableArray<StackMapFrame*>();
> 37:     while(!reader->at_end()) {

Need space between while and (.

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

> 46:     _frame_array = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, StackMapFrame*, _frame_count);
> 47:     for (int i = 0; i < tmp_frame_array->length(); i++) {
> 48:       _frame_array[i] = tmp_frame_array->at(i);

This leaks the GrowableArray for the rest of the Method verification or until a ResourceMark end scope (where also this StackMapFrame array is also recovered).  Can you just change the type of _frame_array to GrowableArray?  It's the same storage and GrowableArray->length() is the correct count that you can use after this.

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

> 142:                                StackMapFrame* init_frame,
> 143:                                u2 max_locals, u2 max_stack, TRAPS) :
> 144:                                _verifier(v), _stream(stream), _code_data(code_data),

Could you indent these out 4 so they don't look like another 4 lines of parameters?

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

> 369:     VerificationType* pre_locals = _pre_frame->locals();
> 370:     int i;
> 371:     for (i=0; i<_pre_frame->locals_size(); i++) {

This needs spaces too since you touched it.  Can "int i" be in scope of the for loop too?

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

> 122: 
> 123:   // Previous and current frame buffers
> 124:   StackMapFrame* _pre_frame;

If you've touched all of the instances of pre_frame, can you rename it prev_frame ?  frame_buf could also be current_frame if you've changed lines that it appears on.

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

> 176:           ErrorContext::bad_stackmap(0, frame),
> 177:           "StackMapTable error: bad offset");
> 178:     }

Rather than have this inlined in the header file, can you put the definition right before where it's used?  I think I only found one place.  I believe this is in only one place.

next_helper should be declared private.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23645#pullrequestreview-2618623415
PR Comment: https://git.openjdk.org/jdk/pull/23645#issuecomment-2660115070
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956616362
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956622314
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956622975
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956624243
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956625449
PR Review Comment: https://git.openjdk.org/jdk/pull/23645#discussion_r1956626899