Add new DDR command constantpool #15404

thallium · 2022-06-22T20:13:53Z

Fix #15128
@fengxue-IS

thallium · 2022-07-06T15:51:16Z

@fengxue-IS ready to be reviewed

fengxue-IS

great work overall, some formatting changes and display message

note: please use (unresolved) instead of NULL

fengxue-IS · 2022-07-12T19:23:40Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+        addCommand("constantpool", "<ramclass>", "dump constant pool for the given ram class.");
+    }
+
+    public J9ClassPointer findClassByName(J9JavaVMPointer vm, String name) throws CorruptDataException {


All helper methods should be private rather than public unless they will be used in other classes

fengxue-IS · 2022-07-12T19:55:14Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                String romInfo = "";
+                String value = "";
+                if (cpType == J9ConstantPool.J9CPTYPE_CLASS) {
+                    cpTypeString = "Class";


be better to have either an map or helper to translate cpType to String and use that directly

fengxue-IS · 2022-07-12T19:58:19Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                        }
+                    }
+                    value += ")";
+                }


please add an else case to catch any CP type that are either incorrect or new type which may be added in future to remind us of updating the tool.

Can you add either a error message or exception throw to make this visible when a unexpected CP type is detected

Throwing Error is inappropriate here: the problem may be a corrupt core file, so CorruptDataException is more likely the right exception.

fengxue-IS · 2022-07-12T20:04:12Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                    romInfo = constantRef.dataEA().getHexAddress();
+                    if (i < ramCPCount) {
+                        ramInfo = ramCPP.getHexAddress();


please put the matching struct type before the address ie !J9RAMSingleSlotConstantRef and !J9ROMSingleSlotConstantRef
same for float/long/double

Is it the same thing for long and double since they uses two slots?

long and double constants don't have an entry in RAM CP, so there is nothing to check/put for ramInfo.
long/double will still use the same 2 slot structure type in ROM CP as J9ROMConstantRef
so should print romInfo = "!j9romconstantref " + romCPP.getHexAddress();

fengxue-IS · 2022-07-12T20:18:06Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                            ramInfo = staticRef.valueOffset().getHexValue(); // resolved static
+                        } else if (ref.flags().gt(ref.valueOffset())) {
+                            ramInfo = ref.valueOffset().getHexValue(); // resolved instance
+                        } else {
+                            ramInfo = "NULL"; // unresolved


add the field type (object/static) info after the offset
so something like
0x000000000000001C (Static offset)

for unresolved entries maybe use (unresolved) to be more clear

fengxue-IS · 2022-07-12T20:58:36Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                                        romInfo = "!j9rommethod " + ROMHelp.J9_ROM_METHOD_FROM_RAM_METHOD(sendMethod).getHexAddress();
+                                    }
+                                } else {
+                                    ramInfo = "Method Index: " + methodIndex.longValue();


This should be VTableOffset:

fengxue-IS · 2022-07-12T20:58:57Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                                    ramInfo = "Method Index: " + methodIndex.longValue();
+                                }
+                            } else { // standard interface method
+                                ramInfo = "Method Index: " + methodIndex.longValue();


ITable Index: here

fengxue-IS · 2022-07-12T20:59:55Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                                ramInfo = "Method Index: " + methodIndex.longValue();
+                            }
+                        } else {
+                            ramInfo = "NULL";


same as before, use (unresolved)

fengxue-IS · 2022-07-12T21:05:38Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                    }
+                    J9ROMNameAndSignaturePointer nameAndSignature = romMethodRef.nameAndSignature();
+                    romInfo = "!j9romnameandsignature " + nameAndSignature.getHexAddress();
+                    value = "invoke/invokeExact";


you should be able to get the name/signature from the ROM info, I think that would be better than having a generic invoke/invokeExact

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

fengxue-IS

lgtm, few minor nit pick

fengxue-IS · 2022-07-14T17:13:11Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+                        }
+                    }
+                    value += ")";
+                }


Can you add either a error message or exception throw to make this visible when a unexpected CP type is detected

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

fengxue-IS

lgtm

fengxue-IS · 2022-07-14T17:49:32Z

@keithc-ca can you please review this, thanks
@tajila FYI

debugtools/DDR_VM/src/com/ibm/j9ddr/AuxFieldInfo29.dat

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

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

debugtools/DDR_VM/src/com/ibm/j9ddr/AuxFieldInfo29.dat

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

keithc-ca · 2022-07-21T19:06:22Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+					} catch (NoSuchFieldException | NoClassDefFoundError e) {
+						throw new CorruptDataException(e);


Please list exceptions alphabetically and add a comment explaining why it's reasonable to classify it as corruption.

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

keithc-ca · 2022-07-25T21:05:16Z

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

+
+public class J9ConstantPoolCommand extends Command {
+	private static final HashMap<Long, String> cpTypeToString = new HashMap<Long, String>();
+	private static final int METHOD_INDEX_OFFSET = 8;


This field is poorly named; perhaps you meant to use J9_ITABLE_INDEX_SHIFT?

It's also used for virtual method, and it's hard-coded elsewhere so I can't come up with a good name.

J9_ITABLE_INDEX_SHIFT is 10, and that should be defined and used in rightShift(10) calls, this value of 8 has always been hardcoded as number in VM, we should look to make that into a constant in the future, in this PR, we can just name it J9_METHOD_INDEX_SHIFT

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

fengxue-IS · 2022-08-08T19:08:02Z

@keithc-ca can this PR be merged or do you have any additional concern that you would like to see addressed?

keithc-ca · 2022-08-09T19:41:29Z

I'll have another look shortly.

keithc-ca

Many of my comments were inappropriately marked as "resolved". I've unresolved the ones I still think need attention.

debugtools/DDR_VM/src/com/ibm/j9ddr/AuxFieldInfo29.dat

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

thallium · 2022-08-09T20:26:09Z

I might somehow messed up with the branch and I'm trying to get it fixed.

fengxue-IS · 2022-08-09T20:51:42Z

FYI 379aa5a seem to be the snapshot before incorrect merge

thallium · 2022-08-09T20:54:56Z

FYI 379aa5a seem to be the snapshot before incorrect merge

yeah I pushed the master branch in my java 19 repo without realizing it's not in sync with my java 18 repo which contains the latest changes.

keithc-ca · 2022-08-11T19:58:34Z

See #15128 (comment).

fengxue-IS · 2022-08-21T22:06:49Z

See #15128 (comment).

Copying reply from #15128 (comment)

This is expected as when a classfile is loaded by the VM, CP entries will be optimized by the JVM (items reordered : ldc entries moved to the top, long and double moved to the end, UTF8 that do not get referenced directly gets removed from constantpool and referenced via SRPs)

fengxue-IS · 2022-08-24T15:37:53Z

@keithc-ca can you take another look

debugtools/DDR_VM/src/com/ibm/j9ddr/CompatibilityConstants29.dat

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

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

thallium · 2022-09-22T13:09:50Z

@keithc-ca Your comments have been addressed

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>

keithc-ca · 2022-09-22T20:30:13Z

jenkins test sanity alinux64 jdk8,jdk17

It was needlessly added in eclipse-openj9#15404. Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>

thallium force-pushed the master branch 4 times, most recently from cd91689 to d17b615 Compare June 29, 2022 20:48

thallium force-pushed the master branch 2 times, most recently from 8a90b4f to c739e04 Compare July 6, 2022 15:29

thallium changed the title ~~[WIP] Add new DDR command constantpool~~ Add new DDR command constantpool Jul 11, 2022

fengxue-IS suggested changes Jul 12, 2022

View reviewed changes

thallium requested a review from fengxue-IS July 13, 2022 18:46

fengxue-IS reviewed Jul 14, 2022

View reviewed changes

thallium force-pushed the master branch from 606d657 to c5c9087 Compare July 14, 2022 17:34

fengxue-IS approved these changes Jul 14, 2022

View reviewed changes

fengxue-IS requested a review from keithc-ca July 14, 2022 17:48

keithc-ca requested changes Jul 14, 2022

View reviewed changes

thallium force-pushed the master branch from c5c9087 to 62ab945 Compare July 21, 2022 15:06

thallium requested a review from keithc-ca July 21, 2022 15:07

keithc-ca requested changes Jul 21, 2022

View reviewed changes

thallium force-pushed the master branch from 62ab945 to 60adcb8 Compare July 25, 2022 20:21

thallium requested a review from keithc-ca July 25, 2022 20:26

thallium force-pushed the master branch from 60adcb8 to 848e016 Compare July 25, 2022 20:39

keithc-ca reviewed Jul 25, 2022

View reviewed changes

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java Outdated Show resolved Hide resolved

thallium force-pushed the master branch from 848e016 to e29b0a6 Compare July 25, 2022 21:02

keithc-ca reviewed Jul 25, 2022

View reviewed changes

thallium force-pushed the master branch from e29b0a6 to 379aa5a Compare July 26, 2022 19:20

thallium requested a review from keithc-ca July 27, 2022 15:22

thallium force-pushed the master branch from 379aa5a to 6f62b31 Compare August 9, 2022 19:36

keithc-ca requested changes Aug 9, 2022

View reviewed changes

thallium force-pushed the master branch from 6f62b31 to e3e7863 Compare August 9, 2022 22:11

thallium requested a review from keithc-ca August 11, 2022 14:03

keithc-ca requested changes Aug 24, 2022

View reviewed changes

thallium force-pushed the master branch 2 times, most recently from e83e1be to f6548fa Compare August 30, 2022 20:52

keithc-ca requested changes Sep 14, 2022

View reviewed changes

thallium force-pushed the master branch 3 times, most recently from 68feade to e1162c6 Compare September 21, 2022 19:31

keithc-ca requested changes Sep 22, 2022

View reviewed changes

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java Outdated Show resolved Hide resolved

...tools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9ConstantPoolCommand.java Outdated Show resolved Hide resolved

add DDR command constantpool

cb5c1cd

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>

thallium force-pushed the master branch from e1162c6 to cb5c1cd Compare September 22, 2022 19:36

keithc-ca approved these changes Sep 22, 2022

View reviewed changes

keithc-ca merged commit cbbacda into eclipse-openj9:master Sep 22, 2022

keithc-ca added a commit to keithc-ca/openj9 that referenced this pull request May 10, 2023

Remove unnecessary compatibility constant J9DescriptionCpTypeShift

08dbdd3

It was needlessly added in eclipse-openj9#15404. Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>

keithc-ca mentioned this pull request May 10, 2023

Remove unnecessary compatibility constant J9DescriptionCpTypeShift #17376

Merged

midronij pushed a commit to midronij/openj9 that referenced this pull request May 24, 2023

Remove unnecessary compatibility constant J9DescriptionCpTypeShift

31422ac

It was needlessly added in eclipse-openj9#15404. Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>

midronij pushed a commit to midronij/openj9 that referenced this pull request Jun 1, 2023

Remove unnecessary compatibility constant J9DescriptionCpTypeShift

988762a

It was needlessly added in eclipse-openj9#15404. Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new DDR command constantpool #15404

Add new DDR command constantpool #15404

thallium commented Jun 22, 2022

thallium commented Jul 6, 2022

fengxue-IS left a comment

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 14, 2022

keithc-ca Jul 14, 2022

fengxue-IS Jul 12, 2022

thallium Jul 13, 2022

fengxue-IS Jul 13, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS Jul 12, 2022

fengxue-IS left a comment

fengxue-IS Jul 14, 2022

fengxue-IS left a comment

fengxue-IS commented Jul 14, 2022

keithc-ca Jul 21, 2022

keithc-ca Jul 25, 2022

thallium Jul 25, 2022

fengxue-IS Jul 26, 2022 •

edited

fengxue-IS commented Aug 8, 2022

keithc-ca commented Aug 9, 2022

keithc-ca left a comment

thallium commented Aug 9, 2022

fengxue-IS commented Aug 9, 2022

thallium commented Aug 9, 2022

keithc-ca commented Aug 11, 2022

fengxue-IS commented Aug 21, 2022

fengxue-IS commented Aug 24, 2022

thallium commented Sep 22, 2022

keithc-ca commented Sep 22, 2022

		} catch (NoSuchFieldException \| NoClassDefFoundError e) {
		throw new CorruptDataException(e);

Add new DDR command constantpool #15404

Add new DDR command constantpool #15404

Conversation

thallium commented Jun 22, 2022

thallium commented Jul 6, 2022

fengxue-IS left a 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

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

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

fengxue-IS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengxue-IS left a comment

Choose a reason for hiding this comment

fengxue-IS commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengxue-IS Jul 26, 2022 • edited

Choose a reason for hiding this comment

fengxue-IS commented Aug 8, 2022

keithc-ca commented Aug 9, 2022

keithc-ca left a comment

Choose a reason for hiding this comment

thallium commented Aug 9, 2022

fengxue-IS commented Aug 9, 2022

thallium commented Aug 9, 2022

keithc-ca commented Aug 11, 2022

fengxue-IS commented Aug 21, 2022

fengxue-IS commented Aug 24, 2022

thallium commented Sep 22, 2022

keithc-ca commented Sep 22, 2022

fengxue-IS Jul 26, 2022 •

edited