[lworld] RFR: 8352068: [lworld] test StrictFinalInstanceFieldsTest.java needs to be updated after fix for JDK-8351951 [v4]

[Dan Heidinga]

On Wed, 21 May 2025 14:44:50 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Updates to javac prevent it from creating classes with improper uses of strict fields. Additionally, non-final strict fields are now allowed so `StrictFinalInstanceFieldsTest.java` needs to updated with new test cases. This patch adds new cases for final and non-final stricts as well as negative test cases using asmtools that check improper use of strict fields and early larval stack map frames. 
>> 
>> During testing, it was revealed that the new verify errors added in [JDK-8354694](https://bugs.openjdk.org/browse/JDK-8354694) do not work properly so a fix is included in this patch.
>> 
>> Note: This cannot be integrated until jtreg is updated with a recent build of asmtools which can handle strict fields and can generate early_larval frames.
>
> 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 seven commits:
> 
>  - Merge branch 'lworld' into new_strict_tests_8352068
>  - Added bug number to problem listed test
>  - Problem list test until jtreg is updated
>  - Remove old test
>  - Added copyright
>  - Added more positive test cases
>  - [lworld] test StrictFinalInstanceFieldsTest.java needs to be updated after fix for JDK-8351951

Maintaining jasm and jcod files can be a pain.  The less those files contain, the better as it is less classfile / bytecode to try to build in your head while debugging.

One option for set of common tests like this that all have x & y fields, as well as getter methods and a complicated toString, is to use an abstract parent class:

abstract JasmParent extends Parent {
  abstract int get_x();
  abstract int get_y();
  
  public String toString() {
          return "x: " + get_x() + "\n" + "y: " + get_y() + "\n" + super.toString();
    }
}

and have all the .jasm file class extend this class instead of using Parent directly.  This will move all the bootstrap methods & indy related code out of jasm files and hopefully result in something simpler to look at.

Another option is to keep Parent as it is, but have its toString() use Reflection like: `this.getClass().getDeclaredFeilds()` to find all the fields and print them.  Any solution that moves code out of jasm / jcod files makes the code easier to maintain and understand longterm

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/BadChild.jasm line 40:

> 38:          putfield          Field y:"I";
> 39:          aload_0;
> 40:          invokespecial     Method Parent."<init>":"()V";

Suggestion:

         invokespecial     Method Parent."<init>":"()V";  // Strict field x must be set before this call

Adding a comment to the code to make the error explicit helps with understanding the test in the future

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/BadChild1.jasm line 40:

> 38:          putfield          Field x:"I";
> 39:          aload_0;
> 40:          invokespecial     Method Parent."<init>":"()V";

Suggestion:

         invokespecial     Method Parent."<init>":"()V";  // Strict field y must be set before this call

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/BadChild1.jasm line 61:

> 59:   }
> 60: 
> 61:   public Method toString:"()Ljava/lang/String;"

Can the toString() implementations be added to the Parent class?  It cuts down on the amount of bytecode that needs to be understood.  Using reflection to look up the fields in the Parent class is a fine way to approach this

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/ControlFlowChildBad.jasm line 51:

> 49:          iconst_2;
> 50:          putfield          Field y:"I";
> 51:          goto              L39;

Suggestion:

         goto              L39;  // Strict field x never set on this path

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/EndsInEarlyLarval.jcod line 167:

> 165:                   7b;                      // same_frame
> 166:                 };
> 167:                 246b, []{}, {              // early_larval_frame, no base frame

Suggestion:

                246b, []{}, {              // ERROR HERE! early_larval_frame, no base frame

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/NestedEarlyLarval.jcod line 162:

> 160:               [] {                         //
> 161:                 246b, []{#8}, {            // early_larval_frame
> 162:                   246b;                    // early_larval_frame, illegal

Suggestion:

                  246b;                    // ERROR HERE: early_larval_frame, nest early_larval is illegal

test/hotspot/jtreg/runtime/valhalla/inlinetypes/verifier/StrictFieldsNotSubset.jcod line 161:

> 159:             Attr(#36) {                    // StackMapTable
> 160:               [] {                         //
> 161:                 246b, []{#8; #12}, {            // early_larval_frame

Is the issue here?  That #12 is not a strict field?  A comment of the form `// ERROR: #12 doesn't name a strict field` would go a long way to making this understandable in the future

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

PR Review: https://git.openjdk.org/valhalla/pull/1449#pullrequestreview-2861806904
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102921671
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102922516
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102925901
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102929268
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102932625
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2102934935
PR Review Comment: https://git.openjdk.org/valhalla/pull/1449#discussion_r2103173386