Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: handle stack frames in the correct order #1762

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andreabergia
Copy link
Contributor

Commit f3c64ee removed ObjArray and replaced its usage with standard JDK classes. In Interpreter, in particular, an ArrayDeque is now used to store
cx.previousInterpreterInvocations, which is used to generate the stack frame information. However, there is one place where toArray is done, and the behavior for ObjArray and ArrayDeque are different (swapped).
Unfortunately no tests actually ends up exercising this difference due to the interpreter function peeling optimization done in #1510.

We have discovered this problem because, in ServiceNow's fork, we currently need to disable the function peeling optimization.

I cannot figure out how to write an unit test for this problem, though. Any ideas?


To verify that the behavior are different, I've written this code:

class ObjArrayTest {
	public static void main(String[] args) {
		var objArray = new ObjArray();
		objArray.push(1);
		objArray.push(2);
		objArray.push(3);
		System.out.println("objArray: " + Arrays.toString(objArray.toArray()));
	
		var deque = new ArrayDeque<Integer>();
		deque.push(1);
		deque.push(2);
		deque.push(3);
		System.out.println("ArrayDeque: " + Arrays.toString(deque.toArray()));
	}
}

It prints:

objArray: [1, 2, 3]
ArrayDeque: [3, 2, 1]

Commit f3c64ee removed `ObjArray` and
replaced its usage with standard JDK classes. In `Interpreter`, in
particular, an `ArrayDeque` is now used to store
`cx.previousInterpreterInvocations`, which is used to generate the
stack frame information. However, there is one place where `toArray`
is done, and the behavior for `ObjArray` and `ArrayDeque` are different
(swapped).
Unfortunately no tests actually ends up exercising this difference due
to the interpreter function peeling optimization done in
mozilla#1510.

We have discovered this problem because, in ServiceNow's fork, we
currently need to disable the function peeling optimization.
@gbrail
Copy link
Collaborator

gbrail commented Dec 20, 2024

I could see how this would be a problem! I think this is a fine solution.

As for testing, is there a way to verify the stack traces in interpreted mode? I'm not totally sure where this stuff happens but I figure that we'd get backwards stack traces in this case, would we not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants