-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Replace conservative overflow check with int limit #121177
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
base: main
Are you sure you want to change the base?
Conversation
| // ESI == element count | ||
| LEAF_ENTRY RhpNewPtrArrayFast, _TEXT | ||
|
|
||
| // Delegate overflow handling to the generic helper conservatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the ceremony with FAST_OBJECT_ARRAY_ALLOCATION_THRESHOLD and delegating the call conservatively, it would be easier to just do the right thing here:
// we want to limit the element count to the non-negative 32-bit int range
cmp rsi, 0x07fffffff
ja LOCAL_LABEL(ArraySizeOverflow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can bring others to the same plan.
|
Tagging subscribers to this area: @mangod9 |
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
| ;; RDX == element count | ||
| LEAF_ENTRY RhpNewPtrArrayFast, _TEXT | ||
|
|
||
| ; Delegate overflow handling to the generic helper conservatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be on the 0x07FFFFFFF plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try that, but it does not have LOCAL_LABEL machinery, and for some reasons label was not found. After writing this I probably should look for typo somewhere.
| LEAF_ENTRY RhpNewPtrArrayFast, _TEXT | ||
| NEW_ARRAY_FAST_PROLOG | ||
|
|
||
| // Delegate overflow handling to the generic helper conservatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be on the conservative 0x40000000 plan, like any other 32-bit platform.
This check needs to ensure that add r2, r2, r1, lsl #2 below won't overflow silently.
|
Nit: PR title could now be |
|
Failing tests: |
| // $a1 == element count | ||
| LEAF_ENTRY RhpNewPtrArrayFast, _Text | ||
|
|
||
| // Delegate overflow handling to the generic helper conservatively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Delegate overflow handling to the generic helper conservatively" should be deleted here and on other 64-bit platforms.
Third item from #116191