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 underestimation of array length in constrainAload #7461

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

dylanjtuttle
Copy link
Contributor

@dylanjtuttle dylanjtuttle commented Sep 16, 2024

To this point, VP has constrained the length of an array in constrainAload to be between 0 and TR::getMaxSigned<TR::Int32>() / elementSize elements long. This was likely a due to a historical limitation. Since arrays can be longer than TR::getMaxSigned<TR::Int32>() bytes, if such an array were copied, it would fail the ArrayCopyBNDCHK, causing erroneous removal of trees that would prevent the copy from being performed. I discussed my findings during my investigation of this issue in a series of comments in openj9 #19247.

This PR increases the high bound of the constraint on the length of an array in constrainAload by using J9::ObjectModel::maxArraySizeInElements() instead, which produces a better upper bound based on the size of the heap. We already use this method in other parts of VP, like constrainArraylength.

Fixes: openj9 #19247, openj9 #19403, openj9 #15500 (most likely)

Increase the upper bound of array length constraint in
constrainAload by taking heap size into account, fixing
erroneous failed ArrayCopyBNDCHK
@dylanjtuttle
Copy link
Contributor Author

@hzongaro Could I get a review on this when you get back?

@dylanjtuttle
Copy link
Contributor Author

Since this is for 0.48, maybe I should get a quicker review. @0xdaryl @vijaysun-omr would one of you be able to review this when you get a chance?

@vijaysun-omr
Copy link
Contributor

Can you please elaborate on the actual problem ? i.e. what was the array info constraint saying originally and how does the fix address it ? Maybe the problem is that TR::getMaxSigned<TR::Int32>()/elementSize underestimates the number of elements because we can have arrays that are more than TR::getMaxSigned<TR::Int32>() bytes long.

@vijaysun-omr
Copy link
Contributor

jenkins build all

@vijaysun-omr vijaysun-omr self-assigned this Sep 16, 2024
@dylanjtuttle
Copy link
Contributor Author

Maybe the problem is that TR::getMaxSigned<TR::Int32>()/elementSize underestimates the number of elements because we can have arrays that are more than TR::getMaxSigned<TR::Int32>() bytes long.

That's correct. I've updated the description with a more in depth explanation of the problem and how this PR will fix it.

@dylanjtuttle dylanjtuttle changed the title Take heap size into account for high bound of array length Fix underestimation of array length in constrainAload Sep 17, 2024
@vijaysun-omr vijaysun-omr merged commit 30130c3 into eclipse-omr:master Sep 17, 2024
16 of 17 checks passed
@dylanjtuttle
Copy link
Contributor Author

@vijaysun-omr Does this need to be double delivered for 0.48? This is the first time I've been in a situation where that might be necessary, so I'm not sure what that entails.

@hzongaro
Copy link
Contributor

hzongaro commented Oct 3, 2024

@pshipton, is it too late for us to get this into the OpenJ9 0.48 release?

@hzongaro
Copy link
Contributor

hzongaro commented Oct 3, 2024

Does this need to be double delivered for 0.48? This is the first time I've been in a situation where that might be necessary, so I'm not sure what that entails.

It would entail opening a pull request for this change against the OpenJ9-OMR 0.48 release branch.

@pshipton
Copy link
Contributor

pshipton commented Oct 3, 2024

is it too late for us to get this into the OpenJ9 0.48 release?

No, everything is delayed.

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

Successfully merging this pull request may close these issues.

Inconsistent execution results after System.arraycopy
4 participants