Ignore null classes when searching the FCC #17001

ehrenjulzert · 2023-03-22T22:18:19Z

Some entries in the FCC have their clazz field set to null, these null classes should not be dereferenced

For #13182 (fixes error in UninitializedInlineFieldsTest, as well as other tests)

ehrenjulzert · 2023-03-22T22:19:51Z

hangshao0 · 2023-03-23T18:57:58Z

runtime/vm/ValueTypeHelpers.cpp

-                        break;
+                J9FlattenedClassCacheEntry *entry = J9_VM_FCC_ENTRY_FROM_FCC(flattenedClassCache, i);
+                if (!J9_VM_FCC_ENTRY_IS_STATIC_FIELD(entry)) {
+                        J9UTF8* currentClassName = J9ROMCLASS_CLASSNAME(entry->clazz->romClass);


There is an existing macro J9_VM_FCC_CLASS_FROM_ENTRY() to get the J9Class. We can then use the J9Class to get the romClass and className.

updated in e9539c9

tajila · 2023-03-24T16:26:02Z

Static fields do not have a class associated with them

In classinitialization.cpp dont we explicitly add the class for static fields

entry->clazz = (J9Class *)((UDATA)valueClass | (UDATA)entry->clazz); //line 454

ehrenjulzert · 2023-03-24T17:46:33Z

Oh yeah I missed that, I only saw this line where the static flag was being set so I assumed there was never any class added

openj9/runtime/vm/createramclass.cpp

Line 2011 in f2971b5

entry->clazz = (J9Class *) J9_VM_FCC_CLASS_FLAGS_STATIC_FIELD;

I guess I should change the code to only ignore the entries where entry->clazz == (J9Class *) J9_VM_FCC_CLASS_FLAGS_STATIC_FIELD, since those are the ones that are actually causing the issue.

tajila · 2023-03-24T20:31:01Z

I guess I should change the code to only ignore the entries where entry->clazz == (J9Class *) J9_VM_FCC_CLASS_FLAGS_STATIC_FIELD, since those are the ones that are actually causing the issue.

Oh interesting, sound it sounds like the failure happens when someone queries it before the class is loaded. Who is the caller when it fails? do you have a stack trace?

ehrenjulzert · 2023-03-24T21:15:11Z

Here's a stack trace:

* thread #35, stop reason = EXC_BAD_ACCESS (code=1, address=0x9)
  * frame #0: 0x00000000023de9bd libj9vm29.dylib`::findJ9ClassInFlattenedClassCache(flattenedClassCache=<unavailable>, className="runtime/valhalla/inlinetypes/Point;\U00000017", classNameLength=34) at ValueTypeHelpers.cpp:149:44 [opt]
    frame #1: 0x00000000023c11df libj9vm29.dylib`ObjectFieldInfo::countInstanceFields(this=0x000070000a887590) at ObjectFieldInfo.cpp:52:28 [opt]
    frame #2: 0x00000000023c97c3 libj9vm29.dylib`::fieldOffsetsStartDo(vm=0x000000000081a020, romClass=<unavailable>, superClazz=0x0000000020038a00, state=0x000070000a887680, flags=30, flattenedClassCache=0x0000000000000004) at resolvefield.cpp:860:13 [opt]
    frame #3: 0x0000000002380f75 libj9vm29.dylib`internalCreateRAMClassFromROMClassImpl(vmThread=<unavailable>, classLoader=0x0000000000893998, romClass=<unavailable>, options=<unavailable>, elementClass=0x0000000000000000, methodRemapArray=0x0000000000000000, entryIndex=-1, locationType=0, classBeingRedefined=0x0000000000000000, superclass=0x0000000020038a00, state=0x000070000a887d90, hostClassLoader=0x0000000000893998, hostClass=0x0000000000000000, module=0x0000000000000000, flattenedClassCache=0x000070000a887eb0, valueTypeFlags=0x000070000a887e50) at createramclass.cpp:2652:19 [opt]
    frame #4: 0x000000000237ead8 libj9vm29.dylib`::internalCreateRAMClassFromROMClass(vmThread=<unavailable>, classLoader=0x0000000000893998, romClass=0x0000000068111ea0, options=<unavailable>, elementClass=0x0000000000000000, protectionDomain=<unavailable>, methodRemapArray=0x0000000000000000, entryIndex=-1, locationType=0, classBeingRedefined=0x0000000000000000, hostClass=0x0000000000000000) at createramclass.cpp:3618:11 [opt]
    frame #5: 0x00000000025e6cd4 libj9vm29.dylib`internalDefineClass(vmThread=<unavailable>, className=<unavailable>, classNameLength=58, classData=<unavailable>, classDataLength=<unavailable>, classDataObject=<unavailable>, classLoader=<unavailable>, protectionDomain=0x00000000ffedc8e0, options=4097, existingROMClass=0x0000000000000000, hostClass=0x0000000000000000, localBuffer=0x000070000a888120) at defineclass.c:175:12 [opt]
    frame #6: 0x000000000274ff24 libjclse29.dylib`defineClassCommon(env=0x000000002023be00, classLoaderObject=<unavailable>, className=<unavailable>, classRep=<unavailable>, offset=8993176, length=<unavailable>, protectionDomain=0x00000000202434e0, options=0x000070000a8882a8, hostClass=0x0000000000000000, patchMap=0x0000000000000000, validateName=1) at jcldefine.c:220:10 [opt]
    frame #7: 0x0000000002742125 libjclse29.dylib`::Java_java_lang_ClassLoader_defineClassImpl(env=0x000000002023be00, receiver=<unavailable>, className=0x0000000020243500, classRep=<unavailable>, offset=<unavailable>, length=<unavailable>, protectionDomain=0x00000000202434e0) at clsldr.cpp:67:18 [opt]
    frame #8: 0x000000004007401f
    frame #9: 0x000000000236e754 libj9vm29.dylib`::sendLoadClass(currentThread=<unavailable>, classLoaderObject=<unavailable>, classNameObject=<unavailable>) at callin.cpp:467:3 [opt]
    frame #10: 0x0000000002379e2b libj9vm29.dylib`internalFindClassInModule at classsupport.c:703:3 [opt]
    frame #11: 0x0000000002379d3c libj9vm29.dylib`internalFindClassInModule [inlined] arbitratedLoadClass(vmThread=0x000000002023be00, className="runtime/valhalla/inlinetypes/UninitializedInlineFieldsTest", classNameLength=58, classLoader=0x0000000000893998, classNotFoundException=<unavailable>) at classsupport.c:904:16 [opt]
    frame #12: 0x0000000002379d30 libj9vm29.dylib`internalFindClassInModule [inlined] loadNonArrayClass(vmThread=<unavailable>, j9module=0x0000000000000000, className="runtime/valhalla/inlinetypes/UninitializedInlineFieldsTest", classNameLength=58, classLoader=0x0000000000893998, options=0, exception=<unavailable>) at classsupport.c:1103:18 [opt]
    frame #13: 0x0000000002379976 libj9vm29.dylib`internalFindClassInModule(vmThread=<unavailable>, j9module=0x0000000000000000, className=<unavailable>, classNameLength=<unavailable>, classLoader=<unavailable>, options=0) at classsupport.c:1148:16 [opt]
    frame #14: 0x0000000002378c33 libj9vm29.dylib`internalFindClassString(currentThread=0x000000002023be00, moduleName=0x0000000000000000, className=<unavailable>, classLoader=<unavailable>, options=0, allowedBitsForClassName=3) at classsupport.c:341:13 [opt]
    frame #15: 0x000000000240baf1 libj9vm29.dylib`VM_BytecodeInterpreterCompressed::run(J9VMThread*) [inlined] VM_BytecodeInterpreterCompressed::inlClassForNameImpl(this=0x000070000a888ae0, _sp=0x000070000a888918, _pc=<unavailable>) at BytecodeInterpreter.hpp:4709:16 [opt]
    frame #16: 0x000000000240b8be libj9vm29.dylib`VM_BytecodeInterpreterCompressed::run(this=0x000070000a888ae0, vmThread=<unavailable>) at BytecodeInterpreter.hpp:10691:3 [opt]
    frame #17: 0x00000000023efa1b libj9vm29.dylib`::bytecodeLoopCompressed(currentThread=<unavailable>) at BytecodeInterpreter.inc:112:21 [opt]
    frame #18: 0x000000000257b052 libj9vm29.dylib`cInterpreter + 22
    frame #19: 0x000000000236fa29 libj9vm29.dylib`::runJavaThread(currentThread=<unavailable>) at callin.cpp:682:4 [opt]
    frame #20: 0x00000000023ead1b libj9vm29.dylib`javaProtectedThreadProc(portLibrary=0x000000000034e1f8, entryarg=0x000000002023be00) at vmthread.cpp:2093:3 [opt]
    frame #21: 0x0000000000720cd0 libj9prt29.dylib`omrsig_protect(portLibrary=0x000000000034e1f8, fn=(libj9vm29.dylib`javaProtectedThreadProc(J9PortLibrary*, void*) at vmthread.cpp:2073), fn_arg=0x000000002023be00, handler=(libj9vm29.dylib`structuredSignalHandler at gphandle.c:613), handler_arg=<unavailable>, flags=<unavailable>, result=0x000070000a888f70) at omrsignal.c:425:12 [opt]
    frame #22: 0x00000000023eac58 libj9vm29.dylib`::javaThreadProc(entryarg=0x000000000081a020) at vmthread.cpp:372:2 [opt]
    frame #23: 0x00000000001c5603 libj9thr29.dylib`thread_wrapper(arg=0x0000000001023050) at omrthread.c:1733:2 [opt]
    frame #24: 0x00007ff8083564e1 libsystem_pthread.dylib`_pthread_start + 125
    frame #25: 0x00007ff808351f6b libsystem_pthread.dylib`thread_start + 15

tajila · 2023-03-28T19:45:28Z

Something seems off, it shouldnt be hitting that code path

		if (J9_ARE_NO_BITS_SET(modifiers, J9AccStatic) ) {
			if (modifiers & J9FieldFlagObject) {
#ifdef J9VM_OPT_VALHALLA_VALUE_TYPES
				J9UTF8 *fieldSig = J9ROMFIELDSHAPE_SIGNATURE(field);
				U_8 *fieldSigBytes = J9UTF8_DATA(J9ROMFIELDSHAPE_SIGNATURE(field));
				if ('Q' == *fieldSigBytes) {
					J9Class *fieldClass = findJ9ClassInFlattenedClassCache(_flattenedClassCache, fieldSigBytes + 1, J9UTF8_LENGTH(fieldSig) - 2);
					U_32 size = (U_32)fieldClass->totalInstanceSize;

We check to make sure its not a static field

tajila · 2023-03-28T19:47:06Z

do you know which class is being loaded?

ehrenjulzert · 2023-03-29T19:26:26Z

The crash occurs when any entry in the FCC just has its clazz set to J9_VM_FCC_CLASS_FLAGS_STATIC_FIELD, it doesn't have to be the class that's being searched for. It looks like the code in countInstanceFields is just checking if the current class being searched for is static.

tajila · 2023-03-29T19:30:37Z

okay I see. I think all we need is a NULL check on J9_VM_FCC_ENTRY_FROM_FCC(flattenedClassCache, i)->clazz

ehrenjulzert · 2023-03-30T15:27:49Z

Ok I fixed the check to just look if the class is null

tajila · 2023-03-30T17:32:11Z

jenkins compile win jdk8

tajila · 2023-03-30T17:32:40Z

Jenkins test sanity,extended xlinuxval jdknext

tajila · 2023-03-30T17:32:54Z

Jenkins test sanity zlinuxvalst jdknext

hangshao0 · 2023-03-30T18:35:45Z

runtime/vm/ValueTypeHelpers.cpp

                }
        }

-        Assert_VM_notNull(clazz);
-        return clazz;
+        Assert_VM_notNull(foundClass);


Is this assertion still valid ?

Yes, if no matching class in the FCC is found then foundClass will be null

ehrenjulzert · 2023-03-31T14:28:47Z

The build failure looks like another extension repo issue, the UpcallStubFactory class gets imported here but it can't be found:

openj9/jcl/src/java.base/share/classes/jdk/internal/foreign/abi/UpcallLinker.java

Lines 35 to 37 in 511fc3d

    
           /*[IF JAVA_SPEC_VERSION >= 21]*/ 
        
           import jdk.internal.foreign.abi.AbstractLinker.UpcallStubFactory; 
        
           /*[ENDIF] JAVA_SPEC_VERSION >= 21 */

I could fix it by changing the guard to /*[IF (JAVA_SPEC_VERSION >= 21) & !INLINE-TYPES]*/, do we want to do that?

hangshao0 · 2023-03-31T14:33:26Z

@ChengJin01 ^^

ChengJin01 · 2023-03-31T16:19:06Z

@ehrenjulzert, there is no need to change any FFI related code in this PR given the compilation failure was due to the out-of-sync issue between the latest JDKnext (ibmruntimes/openj9-openjdk-jdk#572 plus ibmruntimes/openj9-openjdk-jdk#577 which are merged today) and the OpenJDK/Valhalla extension (already obsolete in https://github.com/ibmruntimes/openj9-openjdk-jdk.valuetypes).

@JasonFengJ9, please help synchronize the the OpenJDK/Valhalla extension with the latest changes in JDKnext for JDK21.

ehrenjulzert · 2023-03-31T20:22:18Z

In 05da787 I just removed the NULL assertion at the end of the function and added comments to make it clear that the caller is responsible for checking if the return value is NULL

tajila · 2023-03-31T20:37:59Z

In 05da787 I just removed the NULL assertion at the end of the function and added comments to make it clear that the caller is responsible for checking if the return value is NULL

I think this would be a breaking change, currently all callers expect to get a return. This is used as a shortcut to internalfindClassUTF since the caller is presumably iterating the class instancefieds.

hangshao0 · 2023-03-31T21:09:22Z

It all comes to if it is possible for foundClass to be NULL. Does the change turn the segfault of #17001 (comment) into assertion failure at Assert_VM_notNull(foundClass) ? (if the assertion is kept there)

ehrenjulzert · 2023-03-31T21:26:58Z

All the assertion does is throw an error if a class by the name className is not found in the FCC (which is definitely possible). The crash I originally fixed for this PR occurred while looping over the FCC entries, which is before the assertion.

hangshao0 · 2023-03-31T21:37:08Z

Does the test pass or fail on the assertion if Assert_VM_notNull(foundClass) is kept ?
What is happening might be there is a class that has the same Q type static field and instance field, both are in the FCC. The static entry is added before the instance entry, and the caller is intended to find the instance entry. The null check added in the change skipped the static entry (and prevented the segfault), then eventually find the later entry for the instance field.

JasonFengJ9 · 2023-04-03T13:18:32Z

@ehrenjulzert, there is no need to change any FFI related code in this PR given the compilation failure was due to the out-of-sync issue between the latest JDKnext (ibmruntimes/openj9-openjdk-jdk#572 plus ibmruntimes/openj9-openjdk-jdk#577 which are merged today) and the OpenJDK/Valhalla extension (already obsolete in https://github.com/ibmruntimes/openj9-openjdk-jdk.valuetypes).
@JasonFengJ9, please help synchronize the the OpenJDK/Valhalla extension with the latest changes in JDKnext for JDK21.

OpenJDK valhalla is still at jdk-21+5, there was a merging issue to consume latest openjdk update. Will give it another try.

ehrenjulzert · 2023-04-03T14:40:53Z

After talking to Hang on slack we decided it's better to put the assertion back in

Some entries in the FCC have their clazz field set to null, these null classes should not be dereferenced For eclipse-openj9#13182 (fixes error in UninitializedInlineFieldsTest, as well as other tests) Signed-off-by: Ehren Julien-Neitzert <ehren.julien-neitzert@ibm.com>

JasonFengJ9 · 2023-04-06T15:48:31Z

@ehrenjulzert, there is no need to change any FFI related code in this PR given the compilation failure was due to the out-of-sync issue between the latest JDKnext (ibmruntimes/openj9-openjdk-jdk#572 plus ibmruntimes/openj9-openjdk-jdk#577 which are merged today) and the OpenJDK/Valhalla extension (already obsolete in https://github.com/ibmruntimes/openj9-openjdk-jdk.valuetypes).
@JasonFengJ9, please help synchronize the the OpenJDK/Valhalla extension with the latest changes in JDKnext for JDK21.
OpenJDK valhalla is still at jdk-21+5, there was a merging issue to consume latest openjdk update. Will give it another try.

Merged w/ latest https://github.com/ibmruntimes/openj9-openjdk-jdk (openj9 branch).

tajila · 2023-04-07T14:45:28Z

Jenkins test sanity,extended xlinuxval jdknext

tajila · 2023-04-07T14:45:36Z

Jenkins test sanity win jdk8

tajila · 2023-04-07T14:45:45Z

Jenkins test sanity plinuxvalst jdknext

ehrenjulzert · 2023-04-10T19:08:39Z

This seems to be a known issue (#16779) so it's unrelated to my change, but it does also seem to cause other tests to not run.

hangshao0 reviewed Mar 23, 2023

View reviewed changes

ehrenjulzert force-pushed the UninitializedInlineFieldsTest_fix branch from dfc476c to e9539c9 Compare March 23, 2023 20:59

hangshao0 approved these changes Mar 23, 2023

View reviewed changes

hangshao0 added comp:vm project:valhalla Used to track Project Valhalla related work labels Mar 23, 2023

hangshao0 requested a review from tajila March 23, 2023 22:56

ehrenjulzert force-pushed the UninitializedInlineFieldsTest_fix branch from e9539c9 to 1960bac Compare March 30, 2023 15:24

ehrenjulzert changed the title ~~Ignore static fields when searching the FCC~~ Ignore null classes when searching the FCC Mar 30, 2023

tajila approved these changes Mar 30, 2023

View reviewed changes

hangshao0 reviewed Mar 30, 2023

View reviewed changes

ehrenjulzert force-pushed the UninitializedInlineFieldsTest_fix branch from 1960bac to 05da787 Compare March 31, 2023 20:20

ehrenjulzert force-pushed the UninitializedInlineFieldsTest_fix branch from 05da787 to 3a6ff1e Compare April 3, 2023 15:15

tajila merged commit ef6969e into eclipse-openj9:master Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore null classes when searching the FCC #17001

Ignore null classes when searching the FCC #17001

ehrenjulzert commented Mar 22, 2023 •

edited

ehrenjulzert commented Mar 22, 2023

hangshao0 Mar 23, 2023

ehrenjulzert Mar 23, 2023

tajila commented Mar 24, 2023

ehrenjulzert commented Mar 24, 2023

tajila commented Mar 24, 2023

ehrenjulzert commented Mar 24, 2023

tajila commented Mar 28, 2023 •

edited

tajila commented Mar 28, 2023

ehrenjulzert commented Mar 29, 2023

tajila commented Mar 29, 2023

ehrenjulzert commented Mar 30, 2023

tajila commented Mar 30, 2023

tajila commented Mar 30, 2023

tajila commented Mar 30, 2023

hangshao0 Mar 30, 2023

ehrenjulzert Mar 30, 2023

ehrenjulzert commented Mar 31, 2023

hangshao0 commented Mar 31, 2023

ChengJin01 commented Mar 31, 2023 •

edited

ehrenjulzert commented Mar 31, 2023

tajila commented Mar 31, 2023

hangshao0 commented Mar 31, 2023

ehrenjulzert commented Mar 31, 2023

hangshao0 commented Mar 31, 2023 •

edited

JasonFengJ9 commented Apr 3, 2023

ehrenjulzert commented Apr 3, 2023

JasonFengJ9 commented Apr 6, 2023

tajila commented Apr 7, 2023

tajila commented Apr 7, 2023

tajila commented Apr 7, 2023

ehrenjulzert commented Apr 10, 2023

Ignore null classes when searching the FCC #17001

Ignore null classes when searching the FCC #17001

Conversation

ehrenjulzert commented Mar 22, 2023 • edited

ehrenjulzert commented Mar 22, 2023

hangshao0 Mar 23, 2023

Choose a reason for hiding this comment

ehrenjulzert Mar 23, 2023

Choose a reason for hiding this comment

tajila commented Mar 24, 2023

ehrenjulzert commented Mar 24, 2023

tajila commented Mar 24, 2023

ehrenjulzert commented Mar 24, 2023

tajila commented Mar 28, 2023 • edited

tajila commented Mar 28, 2023

ehrenjulzert commented Mar 29, 2023

tajila commented Mar 29, 2023

ehrenjulzert commented Mar 30, 2023

tajila commented Mar 30, 2023

tajila commented Mar 30, 2023

tajila commented Mar 30, 2023

hangshao0 Mar 30, 2023

Choose a reason for hiding this comment

ehrenjulzert Mar 30, 2023

Choose a reason for hiding this comment

ehrenjulzert commented Mar 31, 2023

hangshao0 commented Mar 31, 2023

ChengJin01 commented Mar 31, 2023 • edited

ehrenjulzert commented Mar 31, 2023

tajila commented Mar 31, 2023

hangshao0 commented Mar 31, 2023

ehrenjulzert commented Mar 31, 2023

hangshao0 commented Mar 31, 2023 • edited

JasonFengJ9 commented Apr 3, 2023

ehrenjulzert commented Apr 3, 2023

JasonFengJ9 commented Apr 6, 2023

tajila commented Apr 7, 2023

tajila commented Apr 7, 2023

tajila commented Apr 7, 2023

ehrenjulzert commented Apr 10, 2023

ehrenjulzert commented Mar 22, 2023 •

edited

tajila commented Mar 28, 2023 •

edited

ChengJin01 commented Mar 31, 2023 •

edited

hangshao0 commented Mar 31, 2023 •

edited