[lworld] RFR: 8281283: Remove usages of __WithField in runtime tests

[David Simms]

On Thu, 17 Feb 2022 19:09:05 GMT, Dan Smith <dlsmith at openjdk.org> wrote:

>> Modifying Valhalla runtime tests to no longer use `__WithField` as a macro for the `withfield` instruction. Most uses can be replaced with standard constructor compilation via javac. The remaining uses are rewritten in jasm.
>> 
>> Summary of changes:
>> 
>> - Remove all uses of `-XDallowWithFieldOperator`
>> - Replace various `make` and `create` factory methods for primitive classes with constructors
>> - Bug fixes for `JumboInline` that were exposed by this change
>> - Rewrote `TestFieldNullability` to use jasm
>> - Rewrote `VWithFieldTest` to use jasm, renamed `WithFieldTest`
>> - Rewrote `WithFieldAccessorTest` to use jasm, made more comprehensive
>> - Deleted `WithFieldNoAccessTest`, made redundant by `WithFieldAccessorTest` changes
>> - Minor tweaks to `TestFieldTypeMismatch` for consistent jasm usage
>> - Cleanup of some debugging output in `asmtools.JtregDriver`
>
> test/hotspot/jtreg/runtime/valhalla/inlinetypes/JumboInline.java line 79:
> 
>> 77:         j = __WithField(j.l1, l1);
>> 78:         j = __WithField(j.l2, l0 + l1);
>> 79:         j = __WithField(j.l3, l1 + l2);
> 
> Bug here (and the following lines): `l2` refers to `this.l2`, not `j.l2`—if `this` is `JumboInline.default`, that value is 0, not `l0+l1`.

Agreed

> test/hotspot/jtreg/runtime/valhalla/inlinetypes/JumboInline.java line 113:
> 
>> 111:             return (l0 == j.l0 && l1 == j.l1 && l2 == j.l2 && l3 == j.l3
>> 112:                     && l4 == j.l4 && l5 == j.l5 && l6 == j.l6 && l7 == j.l7
>> 113:                     && l8 == j.l8 && l9 == j.l9 && l10 == j.l10 && l7 == j.l10
> 
> `l7 == j.l10`? Happens to work, because these are always 0, per the above bug.

The newer code here is cleaner, and more importantly correct. Again, agree, it only worked since "0" default

> test/hotspot/jtreg/runtime/valhalla/inlinetypes/TestFieldTypeMismatch.java line 32:
> 
>> 30:  * @library /test/lib
>> 31:  * @build org.openjdk.asmtools.* org.openjdk.asmtools.jasm.*
>> 32:  * @run driver org.openjdk.asmtools.JtregDriver jasm -strict TestFieldTypeMismatchClasses.jasm
> 
> Naming convention change: suggest putting jasm classes in a file named `[Test name]Classes.jasm`. No need for any classes declared there to actually use that name, although they should have names that won't clash with other tests in this directory.

Good idea

> test/hotspot/jtreg/runtime/valhalla/inlinetypes/TestValue4.java line 48:
> 
>> 46: 
>> 47:     public TestValue4() {
>> 48:         this((int) System.nanoTime());
> 
> Don't need the bit twiddling logic, because it's reproduced by the ByteBuffer reads in the other constructor.

At some point early on, debugging the interpreter was probably easy to not leave the frame, hence the manual bit twiddle, but I think we are past that now, nice clean up.

> test/hotspot/jtreg/runtime/valhalla/inlinetypes/WithFieldAccessorTest.java line 66:
> 
>> 64:         catchAccessError(() -> WithFieldSamePackage.withL(start, 10));
>> 65:         catchAccessError(() -> WithFieldSamePackage.withD(start, 11));
>> 66:         catchAccessError(() -> WithFieldSamePackage.withI(start, 12));
> 
> These calls replace the WithFieldNoAccessTest. (Not checking error messages. Should I add that?)

I think error message was more useful when things were unstable and this test broke more easily. Assume the frame information is correct and offending line number can be determined. Looks good.

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

PR: https://git.openjdk.java.net/valhalla/pull/653