Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer #20740

ThanHenderson · 2024-12-03T19:56:01Z

Signed-off-by: Nathan Henderson [email protected]

ThanHenderson · 2024-12-04T15:04:13Z

Part of addressing: #20612

@babsingh could you review?

We may need to test across multiple GC policies.

babsingh · 2024-12-04T15:35:39Z

runtime/vm/lookupmethod.c

+		J9UTF8 *moduleName = module->moduleName;
+		U_8 *nameData = J9UTF8_DATA(moduleName);
+		UDATA nameLength = J9UTF8_LENGTH(moduleName);
+		nameBuffer = (char *)j9mem_allocate_memory(nameLength + 1, OMRMEM_CATEGORY_VM);


malloc's failure case needs to be addressed.

Missed this. Thanks.

Maybe we can introduce copyJ9UTF8ToUTF8WithMemAlloc, similar to copyStringToUTF8WithMemAlloc, which could be used in most places?

The problem is that when we add the rest of the support for snapshotting modules, we need to allcocate the moduleName J9UTF8s with the snapshot sub-allocator.

The API could just take a function pointer to the allocator to get around that, or we could have a snapshot specific function for when snapshots are enabled.

Note: my above description doesn't apply to where this comment is, but to other paths in this patch.

The API could just take a function pointer to the allocator to get around that

This one seems better (less code). Instead of a function pointer, we can also use a boolean to toggle between the allocators.

we can also use a boolean to toggle between the allocators

To keep it general/reusable beyond snapshotting, I think it is better to have the allocator as a parameter rather than a boolean to switch between the two.

But here, I will use the functions that we already have, and add a copyJ9UTF8ToUTF8WithMemAlloc function as if we were ignorant of the future module snapshotting support. Then in the PR that adds modules to the snapshots, I will add a variant that takes an allocator pointer and use that anywhere we need.

runtime/vm/classsupport.c

runtime/verbose/verbose.c

babsingh · 2024-12-04T15:55:18Z

@dmitripivkine Please review the GC code.

runtime/j9vm/java11vmi.c

dmitripivkine

GC part of the change looks good to me

runtime/util/cphelp.c

runtime/vm/stringhelpers.cpp

runtime/jcl/common/jclexception.cpp

runtime/vm/stringhelpers.cpp

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/ModularityHelper.java

runtime/oti/j9nonbuilder.h

ThanHenderson · 2024-12-07T01:56:42Z

I've addressed the comments.

babsingh · 2024-12-09T15:13:23Z

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/ModularityHelper.java

+	public static String getModuleName(J9ModulePointer modulePtr) throws CorruptDataException {
+	    String moduleName = null;
+	    if (modulePtr != null) {
+	        Object mn = modulePtr.moduleName();
+	        if (mn instanceof J9ObjectPointer) {
+	            moduleName = J9ObjectHelper.stringValue((J9ObjectPointer)mn);
+	        } else if (mn instanceof J9UTF8Pointer) {
+	            moduleName = J9UTF8Helper.stringValue((J9UTF8Pointer)mn);
+	        } else {
+	            throw new CorruptDataException("Invalid module name data type encountered: " + mn.toString());
+	        }
+	    }
+	    return moduleName;


Tabs will need to be used here. We should perform a brief sanity check using jdmpview + core file to confirm that this helper is functioning correctly.

There is something wrong with my auto indent functionality between vim/VSCode; really need to nail that down. Thanks for catching that. I'll test it too.

This function won't work on older core files because J9ModulePointer::moduleName() is generated for the build of jdmpview based on they moduleName type in the JDK build. i.e. the signature is (and from now on will be):

@com.ibm.j9ddr.GeneratedFieldAccessor(offsetFieldName="_moduleNameOffset_", declaredType="J9UTF8*") public J9UTF8Pointer moduleName() throws CorruptDataException {...}

Using an older core file doesn't change the fact that this function returns a J9UTF8Pointer.

On the contrary, the blob in an old core records the fact that moduleName was J9Object*.

$ omr_blob_reader jdk-21/lib/default/j9ddr.dat ... Struct name: J9Module no superName sizeOf: 64 fieldCount: 9 constCount: 0 Field declaredName: moduleName declaredType: struct J9Object* offset: 0

Also, the indentation is a mix of tabs and spaces; here, the indentation should be exclusively tabs.

I'm not sure there is precedent for changing the type of a field used in DDR code. We may need to use a new name for the J9UTF8 flavour. Of course, that means that neither can be a "required" field in the sense used in AuxFieldInfo29.dat.

On the contrary, the blob in an old core records the fact that moduleName was J9Object*

I understand that this is the case. My point was that using a newer jdmpview with the changed type to examine an older core, results in the J9ModulePointer::moduleName() to cause a:

java.lang.NoSuchMethodError: com/ibm/j9ddr/vm29/pointer/generated/J9ModulePointer.moduleName()Lcom/ibm/j9ddr/vm29/pointer/generated/J9UTF8Pointer; (loaded from <Unknown> by J9DDRClassloader for com.ibm.j9ddr.vm29.) called from class com.ibm.j9ddr.vm29.tools.ddrinteractive.ModularityHelper (loaded from <Unknown> by J9DDRClassloader for com.ibm.j9ddr.vm29.).

because of this difference.

Yes, the signature of the reference is baked into ModularityHelper.class.

runtime/vm/lookupmethod.c

runtime/vm/stringhelpers.cpp

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/ModularityHelper.java

keithc-ca · 2024-12-09T20:00:41Z

runtime/j9vm/java11vmi.c

-		vmFuncs->setNativeOutOfMemoryError(currentThread, 0, 0);
+	J9UTF8 *moduleName = fromModule->moduleName;
+	if (NULL != moduleName) {
+		Trc_MODULE_createPackage(currentThread, package, (const char *)J9UTF8_DATA(moduleName), fromModule);


J9UTF8 data isn't necessarily null-terminated: this may need to specify the length. This issues is present multiple places.

I figured these could be skirted since we enforce null-termination of the moduleName J9UTF8 on J9Module creation.

runtime/j9vm/java11vmi.c

runtime/vm/ModularityHashTables.c

runtime/vm/classsupport.c

runtime/vm/stringhelpers.cpp

runtime/oti/j9consts.h

runtime/oti/j9nonbuilder.h

Signed-off-by: Nathan Henderson <[email protected]>

keithc-ca · 2024-12-10T21:26:32Z

Jenkins test sanity alinux jdk21

keithc-ca · 2024-12-10T22:42:19Z

Also tested locally with new system dump and one from 0.48.0.

ThanHenderson force-pushed the j9utf8modulename branch from 743ca0a to f653682 Compare December 3, 2024 21:18

ThanHenderson force-pushed the j9utf8modulename branch from f653682 to 35d786e Compare December 4, 2024 15:17

dmitripivkine added comp:vm comp:gc labels Dec 4, 2024

babsingh reviewed Dec 4, 2024

View reviewed changes

babsingh requested a review from dmitripivkine December 4, 2024 15:54

babsingh reviewed Dec 4, 2024

View reviewed changes

runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved

runtime/j9vm/java11vmi.c Outdated Show resolved Hide resolved

dmitripivkine approved these changes Dec 4, 2024

View reviewed changes

ThanHenderson force-pushed the j9utf8modulename branch 2 times, most recently from b10cc6b to 2cd0300 Compare December 4, 2024 20:53

ThanHenderson mentioned this pull request Dec 4, 2024

Add hiddenInstanceFields to the VM snapshot #20759

Merged

babsingh reviewed Dec 6, 2024

View reviewed changes

runtime/util/cphelp.c Outdated Show resolved Hide resolved

runtime/vm/stringhelpers.cpp Outdated Show resolved Hide resolved

runtime/jcl/common/jclexception.cpp Outdated Show resolved Hide resolved

babsingh reviewed Dec 6, 2024

View reviewed changes

runtime/vm/stringhelpers.cpp Show resolved Hide resolved

ThanHenderson force-pushed the j9utf8modulename branch from 2cd0300 to 4898374 Compare December 6, 2024 21:12

keithc-ca requested changes Dec 6, 2024

View reviewed changes

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/ModularityHelper.java Outdated Show resolved Hide resolved

runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved

ThanHenderson force-pushed the j9utf8modulename branch from 4898374 to 7a386c3 Compare December 7, 2024 01:56

babsingh reviewed Dec 9, 2024

View reviewed changes

runtime/vm/lookupmethod.c Outdated Show resolved Hide resolved

runtime/vm/stringhelpers.cpp Show resolved Hide resolved

runtime/vm/stringhelpers.cpp Outdated Show resolved Hide resolved

runtime/vm/stringhelpers.cpp Show resolved Hide resolved

keithc-ca reviewed Dec 9, 2024

View reviewed changes

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/ModularityHelper.java Outdated Show resolved Hide resolved

keithc-ca requested changes Dec 9, 2024

View reviewed changes

ThanHenderson force-pushed the j9utf8modulename branch 3 times, most recently from 04d5610 to e22d2d6 Compare December 10, 2024 16:41

keithc-ca requested changes Dec 10, 2024

View reviewed changes

runtime/oti/j9consts.h Show resolved Hide resolved

runtime/oti/j9nonbuilder.h Outdated Show resolved Hide resolved

Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer

4ae925f

Signed-off-by: Nathan Henderson <[email protected]>

ThanHenderson force-pushed the j9utf8modulename branch from e22d2d6 to 4ae925f Compare December 10, 2024 20:11

keithc-ca approved these changes Dec 11, 2024

View reviewed changes

keithc-ca merged commit 771463d into eclipse-openj9:master Dec 11, 2024
6 checks passed

ThanHenderson mentioned this pull request Dec 11, 2024

Fix type conversion error in stringhelpers.cpp #20813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer #20740

Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer #20740

ThanHenderson commented Dec 3, 2024 •

edited

Loading

ThanHenderson commented Dec 4, 2024

babsingh Dec 4, 2024

ThanHenderson Dec 4, 2024

babsingh Dec 4, 2024 •

edited

Loading

ThanHenderson Dec 4, 2024 •

edited

Loading

babsingh Dec 4, 2024

ThanHenderson Dec 4, 2024

babsingh commented Dec 4, 2024

dmitripivkine left a comment

ThanHenderson commented Dec 7, 2024

babsingh Dec 9, 2024

ThanHenderson Dec 9, 2024

ThanHenderson Dec 9, 2024

keithc-ca Dec 9, 2024

keithc-ca Dec 9, 2024 •

edited

Loading

ThanHenderson Dec 9, 2024

keithc-ca Dec 9, 2024

keithc-ca Dec 9, 2024

ThanHenderson Dec 9, 2024

keithc-ca commented Dec 10, 2024

keithc-ca commented Dec 10, 2024

Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer #20740

Change J9Module::moduleName from a j9object_t to a J9UTF8 pointer #20740

Conversation

ThanHenderson commented Dec 3, 2024 • edited Loading

ThanHenderson commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

ThanHenderson Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babsingh commented Dec 4, 2024

dmitripivkine left a comment

Choose a reason for hiding this comment

ThanHenderson commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithc-ca commented Dec 10, 2024

keithc-ca commented Dec 10, 2024

ThanHenderson commented Dec 3, 2024 •

edited

Loading

babsingh Dec 4, 2024 •

edited

Loading

ThanHenderson Dec 4, 2024 •

edited

Loading

keithc-ca Dec 9, 2024 •

edited

Loading