[lworld] RFR: 8343846: [lworld] implement spec changes to stack map tables [v2]

[Coleen Phillimore]

On Thu, 27 Feb 2025 16:46:27 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> This patch implements the spec changes needed to realize the ACC_STRICT flag. See the JBS issue for more details. 
>> 
>> The patch is divided across 3 major commits:
>> 1. VM changes for asserting that strict fields are initialized before the super constructor is called
>> 2. Javac changes for generating the stack map table entries
>> 3. Test
>> 
>> Verified with tier 1-3 tests
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Typo corrections and method renaming

This looks really good!  I have a few comment and name suggestions, and was wondering if your Hashtable could be <NameAndSig, bool> where satisfied is the value?

src/hotspot/share/classfile/stackMapFrame.cpp line 205:

> 203: 
> 204:   // Check that assert unset fields are compatible
> 205:   bool compatible = is_unset_fields_compatible(target->assert_unset_fields());

We decided 'is' was more consistent but it does read funny.  Maybe verify_unset_fields_are_compatible ?

src/hotspot/share/classfile/stackMapFrame.hpp line 166:

> 164:   }
> 165: 
> 166:   bool satisfy_unset_field(Symbol* name, Symbol* signature) {

Could you add a comment above this to say when it's called?  Is this called for putfield?

src/hotspot/share/classfile/stackMapFrame.hpp line 177:

> 175:   }
> 176: 
> 177:   bool is_unset_fields_satisfied() {

Also add a comment when this is called - ie. when you get to the super() call?

Maybe also this one could be verify_unset_fields_are_satisfied().

src/hotspot/share/classfile/stackMapFrame.hpp line 200:

> 198:   }
> 199: 
> 200:   bool is_unset_fields_compatible(AssertUnsetFieldTable* target_table) const {

This one is called at branch targets?

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

> 257: 
> 258:       if (!_prev_frame->assert_unset_fields()->contains(tmp)) {
> 259:         log_info(verification)("Field %s%s is not found among initial strict instance fields", name->as_C_string(), sig->as_C_string());

I think you can put a ResourceMark here without breaking everything for the as_C_string() calls and printing.

src/hotspot/share/classfile/verifier.cpp line 731:

> 729:       if (fs.access_flags().is_strict() && !fs.access_flags().is_static()) {
> 730:         NameAndSig new_field(fs.name(), fs.signature());
> 731:         strict_fields->put(new_field, new_field);

For put, you get a pointer back to the new item, so you could save an iteration by having

if (VerifyNoDebts) {
   new_field._satisfied = true;
}

Or can the value of the hashtable be bool satisfied and the key is NameAndSig ?

src/hotspot/share/classfile/verifier.cpp line 743:

> 741:       strict_fields->iterate_all(satisfy_all);
> 742:     }
> 743:   }

The name of this option seems like the opposite of what it does.  Maybe it should be IgnoreAssertUnsetFields ?

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

Changes requested by coleenp (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1373#pullrequestreview-2649017747
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974298262
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974301061
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974300380
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974302160
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974303764
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974312224
PR Review Comment: https://git.openjdk.org/valhalla/pull/1373#discussion_r1974315079