Skip to content

DDR: Fix function call parsing in StackMap - #17278

Merged
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
dnakamura:stackmap
May 11, 2023
Merged

DDR: Fix function call parsing in StackMap#17278
keithc-ca merged 1 commit into
eclipse-openj9:masterfrom
dnakamura:stackmap

Conversation

@dnakamura

@dnakamura dnakamura commented Apr 25, 2023 •
edited by pshipton
Loading

Copy link
Copy Markdown
  • Merge handling of static/non-static calls
  • Fix bug parsing array arguments
  • Simply control flow

Fixes #16804

@dnakamura

dnakamura commented Apr 25, 2023

Copy link
Copy Markdown
Author

The bug in "parsing array arguments" was introduced in #16411. args[i] != 'L' was translated as J9ClassHelper.isRefOrValSignature(args[i]) instead of !J9ClassHelper.isRefOrValSignature(args[i])

@dnakamura dnakamura mentioned this pull request Apr 25, 2023
Closed
@pshipton
pshipton requested review from keithc-ca and tajila April 25, 2023 20:52
@tajila

tajila commented Apr 26, 2023

Copy link
Copy Markdown
Contributor

@ChengJin01 Please review

@tajila
tajila requested a review from ChengJin01 April 26, 2023 13:51
@ChengJin01

ChengJin01 commented Apr 26, 2023 •
edited
Loading

Copy link
Copy Markdown

It seems simply removing the code associated with J9ClassHelper.isRefOrValSignature resolves the issue in DDR but I am wondering whether the changes have any dependency on the previous DDR code specific to Qtype at #16411.
FYI: @hangshao0, @ehrenjulzert

@dnakamura

dnakamura commented Apr 26, 2023

Copy link
Copy Markdown
Author

For clarity, fixing the bug just involves adding a ! to the if condition. Deleting the if statement all together is part of simplifying the control flow (ie moving the check for doubles and longs to the top of the loop).

The bugfix only version of the change:
image

@ehrenjulzert

ehrenjulzert commented Apr 26, 2023

Copy link
Copy Markdown

LGTM, I probably got rid of the ! by accident because I was making a lot of similar changes at once.

@tajila

tajila commented Apr 27, 2023

Copy link
Copy Markdown
Contributor

jenkins test sanity xlinux jdk17

keithc-ca
keithc-ca requested changes Apr 27, 2023
View reviewed changes
Comment thread debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/stackmap/StackMap.java Outdated
Comment thread debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/stackmap/StackMap.java Outdated
@keithc-ca

keithc-ca commented Apr 27, 2023

Copy link
Copy Markdown
Contributor

In your commit message, I expect you meant "simplify" instead of "simply".

@tajila

tajila commented May 3, 2023

Copy link
Copy Markdown
Contributor

@keithc-ca Please take another look at these changes

keithc-ca
keithc-ca reviewed May 5, 2023
View reviewed changes
Comment thread debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/j9/stackmap/StackMap.java Outdated
Devin Nakamura
DDR: Fix function call parsing in StackMap
c917f46 
- Merge handling of static/non-static calls
- Fix bug parsing array arguments
- Simplify control flow

Signed-off-by: Devin Nakamura <devinn@ca.ibm.com>
@keithc-ca

keithc-ca commented May 11, 2023

Copy link
Copy Markdown
Contributor

Jenkins test sanity alinux64 jdk20

@keithc-ca
keithc-ca merged commit 09a3602 into eclipse-openj9:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tajila tajila tajila approved these changes
@keithc-ca keithc-ca keithc-ca approved these changes
+2 more reviewers
@ChengJin01 ChengJin01 ChengJin01 approved these changes
@ehrenjulzert ehrenjulzert ehrenjulzert approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

comp:vm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DDR !stackslots command is broken at "Pending stack starting at"

6 participants

@dnakamura @tajila @ChengJin01 @ehrenjulzert @keithc-ca @pshipton