Add DDR command continuationstack

[thallium]

continuationstackslots should also work

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

Related: #15181

[thallium]

@fengxue-IS FYI

[fengxue-IS]

Do we still need this if all callers have to specify either a walkThread or ContinuationAddress?

[fengxue-IS]

stubELS is not defined as a pointer, you will have to either update the definition in j9nonbuilder to make it a pointer and allocate it in stdinit.c or jclcinit.c. Or get the actual address here.

[thallium]

You mean change to entryLocalStorageEA instead? I remember entryLocalStorage and entryLocalStorageEA actually do the same thing if ELS is not a pointer, otherwise I'll have a runtime error.

[fengxue-IS]

something similar to jitGPRs

[thallium]

Could you elaborate? I still think entryLocalStorage() returns the address of the ELS after went through some code in dtfj. What I did on jitGPRs is just one extra casting due to different type in J9VMContinuation and in J9VMEntryLocalStorage

[fengxue-IS]

nvm, you are right, I was looking at the generated code for non-struct fields which directly return values, for struct fields nonNullFieldEA is used to return the address

[fengxue-IS]

I would prefer if you can set these fields in the wrapper's constructor, this avoid caller code having to modify the wrapper field directly, so future changes to the wrapper behaviour will be easier.

[fengxue-IS]

lgtm

[fengxue-IS]

@keithc-ca can you please take a look

[fengxue-IS]

@thallium can you fix the line ending

[thallium]

fixed

[fengxue-IS]

@keithc-ca This PR is ready for final review, can you take a look

[keithc-ca]

This should use J9VMContinuationPointer directly by adding in AuxFieldInfo29.dat declarations of the fields we need:

J9VMContinuation.arg0EA = UDATA*
J9VMContinuation.decompilationStack = J9JITDecompilationInfo*
J9VMContinuation.j2iFrame = UDATA*
J9VMContinuation.jitGPRs = UDATA[] /* actually: J9JITGPRSpillArea */
J9VMContinuation.literals = J9Method*
J9VMContinuation.oldEntryLocalStorage = J9VMEntryLocalStorage*
J9VMContinuation.pc = U8*
J9VMContinuation.sp = UDATA*

Use of that class may throw NoClassDefFoundError and uses of the related field accessor functions may throw NoSuchFieldException, both of which will need to be handled. ContinuationStackWalkCommand checks that the core is from a VM that should support continuations, so those exceptions signal an inconsistent dump.

I'd actually prefer that we don't introduce J9VMEntryLocalStoragePointerWrapper: can we replace the field

	public J9VMEntryLocalStoragePointer walkedEntryLocalStorage;

in WalkState with the necessary components instead?

[fengxue-IS]

This should use J9VMContinuationPointer directly by adding in AuxFieldInfo29.dat declarations of the fields

The VM structure struct J9VMContinuation is only defined for Java 19+, including it directly will cause compile issues for Java 8/11/17

can we replace the field in WalkState with the necessary components instead?

The J9VMEntryLocalStoragePointer is using a linked-list structure, we would not be able to directly place the fields into WalkState without heavily modifying the existing code to account for how to iterate the list

[keithc-ca]

The additions I listed for AuxFieldInfo29.dat will make that type available to DDR_VM code.

The approach using J9VMEntryLocalStoragePointerWrapper just seems wrong to me. Perhaps Supplier<J9VMEntryLocalStoragePointer> is the interface we should use. The variant for a thread reads the field from the thread, and the variant for a continuation gets it via a J9VMContinuationPointer.

[fengxue-IS]

The additions I listed for AuxFieldInfo29.dat will make that type available to DDR_VM code.

I wasn't sure if adding type after field declarations would make them available to DDR, if that is the case, we can switch to direct access under a try/catch block when reading the fields.

The approach using J9VMEntryLocalStoragePointerWrapper just seems wrong to me. Perhaps Supplier<J9VMEntryLocalStoragePointer> is the interface we should use. The variant for a thread reads the field from the thread, and the variant for a continuation gets it via a J9VMContinuationPointer.

The reasoning behind using J9VMEntryLocalStoragePointerWrapper is to minimize the code changes on existing stack walker code and to keep it as similar to the VM's walkStackFrames() API as possible to make future maintenance easier. Changing to use Supplier<T> interface would require refactor on all code access to ELS right?

[keithc-ca]

I don't see that J9VMEntryLocalStoragePointerWrapper is needed at all (nor is using Supplier<?>).

StackWalker can simply use this for a continuation:

    walkState.walkedEntryLocalStorage = continuation.oldEntryLocalStorage();

[keithc-ca]

Perhaps the right container is WalkState: we either initialize that state from a thread or a continuation. Advancing from one stack frame to the next is something that should be associated with that state.

[fengxue-IS]

I thought we have agreed on moving the ELS data to an interface that have a different impl for J9VMEntryLocalStorage and J9VMContinuation as per #15181 (comment).

also for the linked list of oldEntryLocalStorage, putting states inside WalkState will cause complication when iterating through the linked list

[keithc-ca]

Yes, I suggested an interface that would allow access to ELS and whatever other information needed to walk stacks. That interface is not ELS, but it seems reasonable to me to adjust WalkState to capture what is needed so navigation from one stack frame to the next can occur. Rather than require two pieces of information (WalkState and something else), the stack walker would use one (WalkState). I think part of the difficulty is that the fields of WalkState are public and modified all over the place - a mistake in my opinion. Rather than perpetuate and expand on that mistake, I'm suggesting we take the time to improve the situation (perhaps even the native implementation as well).

[fengxue-IS]

I think we should try to have this version merged as a baseline on meeting functional expectation.

Refactoring WalkState to an improved design of how stack walking is done should be pursued as a new issue. I don't want to delay the loom features due to redesign / test on a rework of DDR stack walker (potentially even the native impl in swalk.c).

[keithc-ca]

I disagree. From my perspective, this is not ready, and I don't think this DDR portion interferes with meeting functional expectations. It can wait until we have an appropriate implementation.

[thallium]

Reworked using the API from #16356.

[thallium]

@fengxue-IS @keithc-ca your comments have been addressed.

[thallium]

@keithc-ca All your concerns have been addressed.

[keithc-ca]

Jenkins test sanity zlinux jdk21

[pshipton]

Seems this change has caused a test problem #17962
Unless it can be fixed today I'll revert so we don't get all platform all version test failures in the nightly builds.

@keithc-ca @thallium

[thallium]

@pshipton We're not storing walkedEntryLocalStorage in the walk state anymore so the logging part was removed. I could add the field back to the walk state so the failure should be fixed.

[pshipton]

The other option is to remove the test. @TobiAjila @gacholio which do you prefer?

[keithc-ca]

I don't think it makes sense to put back walkedEntryLocalStorage, so I think the test needs to be either updated or removed.

[pshipton]

Doesn't seem we'll fix it in time today so I'm temp reverting this to avoid the large number of failures that will occur.

Reverted in #17963

[tajila]

Im in favour of removing parts that refer to ELS in the walkstate

@thallium I think we only need to remove this part from the test in /openj9/test/functional/DDR_Test/src/j9vm/test/ddrext/junit/TestThread.java

		/* test !stackslots thread,sp,a0,pc,literals,els */
		String paramSet2 = getStackSlotsParamSet(stackSlotsOutput,
				threadAddress, true);
		if (paramSet2 == null) {
			fail("Failed to construct parameter list for command : !stackslots thread,sp,a0,pc,literals,els\nStackSlots Output being used : \n"
					+ stackSlotsOutput);
		} else {
			String stackSlotsOutputParam2 = exec(Constants.STACKSLOTS_CMD,
					new String[] { paramSet2 });
			assertTrue(validate(stackSlotsOutputParam2,
					Constants.STACKSLOTS_SUCCESS_KEY,
					Constants.STACKSLOTS_FAILURE_KEY, true));
		}

[gacholio]

Agreed, as long as we still print the new ELS value when we cross the call-in boundary.