-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
direct_mapping: fix iter_memory_pair_chunks in reverse mode #34204
direct_mapping: fix iter_memory_pair_chunks in reverse mode #34204
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #34204 +/- ##
=======================================
Coverage 81.9% 81.9%
=======================================
Files 819 819
Lines 219302 219320 +18
=======================================
+ Hits 179619 179708 +89
+ Misses 39683 39612 -71 |
32ad13b
to
a9bb35d
Compare
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
a9bb35d
to
4fe02b5
Compare
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.
Looks good to me! There are still some weird edge cases with copies going over the gaps in the stack but that's a more general issue with the gap implementation and not this code.
Thanks for reviewing! Yes we're aware of those, the plan is to disable gaps when we enable direct mapping. |
4fe02b5
to
fde020d
Compare
I can't really talk on the actual code changes here, but just chiming in to confirm that this does resolve the memmove issue we were running into with direct mapping on 1.16.x and all existing 1.17.x versions. Here's the minimal example program we used to test it: https://beta.solpg.io/65626e1afb53fa325bfd0c34. The "Can do overlapping memmove" test was failing before this PR. |
iter_memory_pair_chunks was iterating regions in reverse, but not memory _within_ regions in reverse. This commit fixes the issue and simplifies the implementation by removing nested loops which made control flow hard to reason about.
fde020d
to
bb9aa00
Compare
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.
Some tests and variables should be renamed, as discussed on Slack. Can happen in a separate PR.
iter_memory_pair_chunks was iterating regions in reverse, but not memory _within_ regions in reverse. This commit fixes the issue and simplifies the implementation by removing nested loops which made control flow hard to reason about. (cherry picked from commit 0908882)
…ackport of #34204) (#34236) direct_mapping: fix iter_memory_pair_chunks in reverse mode (#34204) iter_memory_pair_chunks was iterating regions in reverse, but not memory _within_ regions in reverse. This commit fixes the issue and simplifies the implementation by removing nested loops which made control flow hard to reason about. (cherry picked from commit 0908882) Co-authored-by: Alessandro Decina <[email protected]>
iter_memory_pair_chunks was iterating regions in reverse, but not memory within regions in reverse, which broke some overlapping memmoves.
This commit fixes the issue and simplifies the implementation by removing nested loops which made control flow hard to reason about.
I've added (a lot of) tests that compare libc's memmove against our memmove.