-
Notifications
You must be signed in to change notification settings - Fork 554
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
ARM: drutil_insert_get_mem_addr
no longer works correctly on AArch64
#5498
Comments
This may have resulted in incorrect addresses in AArch64 traces gathered since Jan 18. |
This is rather disturbing. What can we do to avoid such regressions in the future?
|
The |
It is! Adding more comprehensive regression tests for In general, are there areas of DR core and API code which rely heavily on the correctness of generated instructions which should also have more comprehensive test coverage? Can we identify a set of Can we identify these code paths and improve error trapping with e.g. Does #2443 cover enough of this? |
So the #5283 fix itself was addressing incorrect addresses which I don't remember realizing when that fix went in: so older traces pre-Jan 18 also had a source of inaccuracies. |
Could this be quantified? If we want to search for load/store instructions whose addresses were computed incorrectly prior to the #5283 fix what would we search for? Is it that any scaled index register was treated as not scaled at all? |
Yes, it's correlated with the extend- if the extend is UXTW or SXTW then it should be a W register , if it is SXTX or LSL/UXTX, it should be an X register |
But the extend scale is also incorrect, right? In the diff for PR #5283 I see things like this:
And now it's:
So a trace gathered prior to that PR that had a store with a scale would have incorrectly ignored the scale and computed the wrong address? (I think |
Indeed, it is incorrect, but it as at least consistently incorrect. UXTX only appears where it should be LSL, and the correct registers can be derived from that if we want to find where the decode errors occurred pre #5283. I'm not sure on the difference between uxtx #3 and lsl #3. I'll have to defer to @AssadHashmi on that |
My understanding is that
I don't think that's the case. The values used to create memref with |
OK, so a trace prior to PR #5283 would only have an incorrect address related to that PR's changes if it had an X register as an index that really should have been a W and it had a non-zero value in the upper bits? The scaling and extension changes from that PR are purely cosmetic. The PR description of I did a sanity test: After PR #5283:
Going back before the PR:
Assuming my ASLR-disabling test will get the same stack address (sure seems like it; too lazy to create a different constant address instead of using SP in a tiny asm test; maybe should have used a .text address and a load) it does seem that the |
- Add client test case using W28 index register in memory instr. - Add FIXME comment and an 'if (index == DR_REG_W28)' check in drutil_insert_get_mem_addr_arm(). This avoids silent errors. There will be a following PR for the actual fix. In the meantime, the test has been added to the known failures list in runsuite_wrapper.pl's %ignore_failures_64. Issue: #5498
- Handle stolen register in drutil_insert_get_mem_addr_arm() if it's used as a 32 bit index register. - Add client test case using W28 as index register in memory instructions. Issue: #5498
- Handle stolen register in drutil_insert_get_mem_addr_arm() if it's used as a 32 bit index register. - Add client test case using W28 as index register in memory instructions. Issue: #5498
Through unrelated validation of virtual-to-physical address gathering, we discovered a source of incorrect addresses in our traces: instructions with Creating a test case we can see that the addresses are wrong:
I ran it without ASLR which just happens to not crash on that memref:
That recorded address 0x0000ffffc0187040 sure looks wrong. It should be:
Instru:
It's not restoring the app's x28 value, so the w28 it uses is DR's. So all aarch64 traces have had incorrect addresses for w28 index registers, all this time. |
The fix in PR #5511 for drutil_insert_get_mem_addr() on a w28 index register failed to use the application value in a temp register and resulted in an incorrect recorded address. The test added in that PR failed to actually check the value and so failed to catch the error. Here we augment the test to check the value by passing the recorded address and the operand to a clean call. This new test fails for the w28 case. With the included fix, it now passes. Fixes #5498
#5283 introduced a change to decoding of load instructions, as a result of that change load index registers can be decoded as
w
registers (as opposed tox
registers as previously).Not all places in DR are prepared to encounter a
w
register in a load index context.For example, this logic in
drutil_insert_get_mem_addr_arm
now doesn't recognize
w
index
registers as stolen, becausedr_get_stolen_reg()
typically returns anx
register.This results in instrumentation being silently incorrect and
drutil_insert_get_mem_addr_arm
producing incorrect addresses.Some other places besides
drutil_insert_get_mem_addr_arm
may also be affected by this change.The text was updated successfully, but these errors were encountered: