Valhalla tests should use migrated value classes

theresa-m commented 1 month ago

Jep 401 migrates existing ValueBased classes (such as java/lang/Integer) to be value classes. Right now these classes are compiled into jdk/lib/valueclasses/java.base-valueclasses.jar and can be used by patching them into the java.base module --patch-module java.base=jdk/lib/valueclasses/java.base-valueclasses.jar

To complete this issue:

[ ] Update Valhalla tests to use ValueBased classes and address test failures
[ ] Add tests to verify that migrated classes are being treated as value classes: a = new Integer(1); b= new Integer(1); a == b should be true. a and b should have the same hashcode. synchronized(a) is not allowed. a.getClass().isValue() should be true.
[ ] Verify that the updates would work on a grinder job
[ ] Add patch-module option to OpenJDK tests https://github.com/eclipse-openj9/openj9/issues/20404 and https://github.com/eclipse-openj9/openj9/issues/13182. Remove from any individual test files. Its causing a segfault that needs to be investigated.

github-actions[bot] commented 1 month ago

Issue Number: 20386 Status: Open Recommended Components: comp:test, comp:vm, comp:build Recommended Assignees: hangshao0, jasonfengj9, theresa-m

theresa-m commented 2 weeks ago

This is currently blocked by https://github.com/eclipse-openj9/openj9/issues/20372

theresa-m commented 6 days ago

There is another MethodHandles error when trying to enable the use of classes in java.base-valueclasses.jar:

Exception in thread "main" java.lang.InternalError: java.lang.NoSuchFieldException: no such field: java.util.concurrent.atomic.Striped64.base/long/getField
    at java.base/jdk.internal.invoke.MhUtil.findVarHandle(MhUtil.java:62)
    at java.base/jdk.internal.invoke.MhUtil.findVarHandle(MhUtil.java:52)
    at java.base/java.util.concurrent.atomic.Striped64.<clinit>(Striped64.java:381)
    at java.base/java.util.concurrent.ConcurrentSkipListMap.addCount(ConcurrentSkipListMap.java:439)
    at java.base/java.util.concurrent.ConcurrentSkipListMap.doPut(ConcurrentSkipListMap.java:683)
    at java.base/java.util.concurrent.ConcurrentSkipListMap.putIfAbsent(ConcurrentSkipListMap.java:1789)
    at java.base/java.util.concurrent.ConcurrentSkipListSet.add(ConcurrentSkipListSet.java:240)
    at java.base/java.net.InetAddress$NameServiceAddresses.get(InetAddress.java:1193)
    at java.base/java.net.InetAddress.getAllByName0(InetAddress.java:1812)
    at java.base/java.net.InetAddress.getLocalHost(InetAddress.java:1925)
    at org.testng.reporters.JUnitXMLReporter.generateReport(JUnitXMLReporter.java:152)
    at org.testng.reporters.JUnitXMLReporter.onFinish(JUnitXMLReporter.java:112)
    at org.testng.TestRunner.fireEvent(TestRunner.java:772)
    at org.testng.TestRunner.afterRun(TestRunner.java:741)
    at org.testng.TestRunner.run(TestRunner.java:509)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:455)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415)
    at org.testng.SuiteRunner.run(SuiteRunner.java:364)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1208)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1137)
    at org.testng.TestNG.runSuites(TestNG.java:1049)
    at org.testng.TestNG.run(TestNG.java:1017)
    at org.testng.TestNG.privateMain(TestNG.java:1354)
    at org.testng.TestNG.main(TestNG.java:1323)
Caused by: java.lang.NoSuchFieldException: no such field: java.util.concurrent.atomic.Striped64.base/long/getField
    at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:936)
    at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1013)
    at java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:3744)
    at java.base/java.lang.invoke.MethodHandles$Lookup.findVarHandle(MethodHandles.java:3192)
    at java.base/jdk.internal.invoke.MhUtil.findVarHandle(MhUtil.java:60)
    ... 26 more
Caused by: java.lang.NoSuchFieldError
    at java.base/java.lang.invoke.MethodHandleNatives.resolve(Native Method)
    at java.base/java.lang.invoke.MemberName$Factory.resolve(MemberName.java:981)
    at java.base/java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1010)
    ... 29 more

hangshao0 commented 6 days ago

The native implementation of MethodHandleNatives.resolve() is Java_java_lang_invoke_MethodHandleNatives_resolve(). The NoSuchFieldError is likely from: https://github.com/eclipse-openj9/openj9/blob/2583a83f21350465715f015fb407481112063c94/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L1394. You can look at why we entered the exception case. May be declaringClass is NULL.

theresa-m commented 5 days ago

I created a small program to reproduce this and its not related to migrated value classes, and only happens when an abstract class inherits from an abstract value class. Also finding classes seems to work but primitives are failing.

I've confirmed that the field is found but the offset is 0 https://github.com/eclipse-openj9/openj9/blob/2583a83f21350465715f015fb407481112063c94/runtime/vm/resolvefield.cpp#L224.

The result never gets set here because the offset (which eventually sets the value of target) is 0. https://github.com/eclipse-openj9/openj9/blob/2583a83f21350465715f015fb407481112063c94/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L1383

The offset is 8 when CAbsVal is not a value class.

import java.lang.invoke.MethodHandles;
import jdk.internal.invoke.MhUtil;
class Test {
        public static void main(String[] args) throws Throwable {
                C m = new C();
        }
}

// error happens when this is a value class
abstract value class CAbsVal {}

abstract class CAbs extends CAbsVal {
        Object o;
        long l;
        static {
                MethodHandles.Lookup l1 = MethodHandles.lookup();
                MhUtil.findVarHandle(l1, "o", Object.class); // this is okay
                MhUtil.findVarHandle(l1, "l", long.class); // this fails
        }
}

class C extends CAbs {}

hangshao0 commented 5 days ago

For value classes, there is no lockword between the object header and the first field. So it is possible that the first field starts at offset 0.

MhUtil.findVarHandle(l1, "o", Object.class); // this is okay MhUtil.findVarHandle(l1, "l", long.class); // this fails

Are you running with -Xnocompressedrefs ? OpenJ9 will put field long l before Object o in -Xnocompressedrefs mode. In compressedrefs mode, field Object o will be put before long l, so I expect MhUtil.findVarHandle(l1, "o", Object.class) to fail.

theresa-m commented 4 days ago

I see, thanks. Yes I'm running with -Xnocompressedrefs. I tried with a compressed refs build and it does fail on Object o.

eclipse-openj9 / openj9

Valhalla tests should use migrated value classes #20386