On Mon, 31 Aug 2026 14:33:28 GMT, Matias Saavedra Silva <[email protected]> wrote:
>> Early_larval frames can only apply to frames with the uninitializeThis flag >> set, however, this rule was not properly being enforced. Instead, the >> verifier looked at the state of uninitializedThis for the previous frame, so >> this patch moves this check until after the nested frame has been parsed. >> Verified with a new regresson test case and tier 1-5 tests. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Matias Saavedra Silva has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 11 commits: > > - Removed satisfy_unset_field error > - Merge branch 'master' of github.com:openjdk/jdk into > early_larval_verifier_8390256 > - Removed unused flag, moved AssertUnsetFields definition, aligned change to > spec > - Test fix and cleanup > - Dan offline comments > - Added tests and fixes for oddly ordered frames > - Fixed crash > - Cleanup > - Added test and fix for unusual frame ordering > - Updated test > - ... and 1 more: https://git.openjdk.org/jdk/compare/f8c6117c...f7c0a206 src/hotspot/share/classfile/stackMapFrame.cpp line 242: > 240: // 4. Source and target unset fields are non-null > 241: // We are merging from one frame with unset strict fields > information to another > 242: // and must ensure the unset fields lists are compatible. I would drop this comment as it describes 4 cases but only handles some of them. The better place to handle all the cases is in `verify_unset_fields_compatibility`. I'd also remove the `if ((assert_unset_fields() != nullptr) || (target->assert_unset_fields() != nullptr)) {` check as that's trivially handled by the first case in `verify_unset_fields_compatibility` where the code returns true if assert_unset_fields() == nullptr src/hotspot/share/classfile/stackMapFrame.hpp line 171: > 169: if (table == nullptr) { > 170: _assert_unset_fields = nullptr; > 171: } else { `copy_unset_fields` already handles the nullptr case so we don't need to handle it here as well. Just pass `table` to copy_unset_fields unconditionally src/hotspot/share/classfile/stackMapFrame.hpp line 177: > 175: > 176: // Called when verifying putfields to mark strict instance fields as > satisfied > 177: void satisfy_unset_field(Symbol* name, Symbol* signature) { Suggestion: void satisfy_unset_field(Symbol* name, Symbol* signature) { // The verifier creates the initial set of strict instance fields and // validates the set of strict fields named in early_larval frames // so there is no way to have a non-strict field in the set. We // can unconditionally remove fields here, regardless of whether // they are strict or not, or have been removed already, as the // easiest and safest implementation. src/hotspot/share/classfile/stackMapTable.cpp line 296: > 294: > 295: u2 num_unset_fields = _stream->get_u2(CHECK_NULL); > 296: AssertUnsetFieldTable* new_fields = new AssertUnsetFieldTable(); Suggestion: AssertUnsetFieldTable* new_fields = nullptr; if (num_unset_fields != 0) { new_fields = new AssertUnsetFieldTable(); ..... } _assert_unset_fields_buffer = new_fields; We can avoid allocating the new_fields table here if the early_larval frame explicitly lists 0 fields. src/hotspot/share/classfile/verifier.cpp line 736: > 734: > 735: // Collect the initial strict instance fields if there are any > 736: AssertUnsetFieldTable* strict_fields = new AssertUnsetFieldTable(); Can we move the allocation of the `strict_fields` table into the `if (m->is_object_constructor())` case? This way only constructors have to allocate the table, not all methods. src/hotspot/share/classfile/verifier.cpp line 741: > 739: if (fs.access_flags().is_strict() && > !fs.access_flags().is_static()) { > 740: NameAndSig new_field(fs.name(), fs.signature()); > 741: strict_fields->put(new_field, true); We could also move the `strict_fields` allocation down to here by doing: if (strict_fields == nullptr) { strict_fields = new AssertUnsetFieldTable(); } strict_fields->put(new_field, true); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3895951358 PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3895974380 PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3896022327 PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3896146889 PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3896189734 PR Review Comment: https://git.openjdk.org/jdk/pull/32459#discussion_r3896195781
