i#3801,#3823: Make drreg state restoration more robust

[abhinav92003]

Revamps the drreg state restoration logic in drreg_event_restore_state to correctly
handle various corner cases like multiple restores (i#4939), aflags in reg (i#4933)
and multi-phase use (i#3823).

Makes the reconstructed ilist available to drreg_event_restore_state. This is needed
for determining whether some instr is app or tool.

The new logic looks at the complete ilist to track the movement of the app value of
gpr/aflags across spills and restores. The native value can be in the gpr/aflags
itself or one or more spill slots.
We do not forget a spill slot on seeing one restore (which caused i#4939), but
remember that the app value can still be found in the spill slot, in addition to
the gpr itself. Instead, a spill slot is forgotten when it is used to spill some
other reg.
We also track whether gprs/aflags contain their native app value or not. This helps
in handling cases of overlapping or nested spill regions arising out of multi-phase
use of drreg. Tool writes (except restores) to gprs/aflags clobber the native value.
App writes to gprs/aflags install a native value and invalidate the value in the
reg's spill slot. Restores from spill slots also install a native value.
We also track movement of aflags to registers more robustly (to fix i#4933).

Adds tests for the i#4939, i#4933, i#3823 (gpr case, not aflags). Test for i#3823
aflags case will be added in PR #4917.

Issue: #4933, #4939, #3823, #3801
Fixes: #4933
Fixes: #4939

[abhinav92003]

Still working on root-causing the failures on drx-test in AArch64:

<Internal Error: Failed to encode instruction: 'b      $0x0000ffff90bd9da0'
>

This seems to occur during the instr_get_eflags(inst, DR_QUERY_INCLUDE_ALL) call in drreg_event_restore_state, for the above-mentioned instr. The instr is part of the syscall mangling jumps, and the target looks slightly different than other branch instrs: b @0x0000fffd40a0bec0[8byte] vs b $0x0000ffff846aa22c

[derekbruening]

Still marked as Draft

[derekbruening]

Comments from 1st two files (didn't look at drreg yet).

[abhinav92003]

Still working on root-causing the failures on drx-test in AArch64:

<Internal Error: Failed to encode instruction: 'b      $0x0000ffff90bd9da0'
>

This seems to occur during the instr_get_eflags(inst, DR_QUERY_INCLUDE_ALL) call in drreg_event_restore_state, for the above-mentioned instr. The instr is part of the syscall mangling jumps, and the target looks slightly different than other branch instrs: b @0x0000fffd40a0bec0[8byte] vs b $0x0000ffff846aa22c

The encoding error itself seems because the immediate is too big for b (which can handle 28-bit offset only). private_instr_encode tries to encode the branch instr in some buf and the offset (target - buf-address) is perhaps more than 28-bit.

dynamorio/core/ir/instr_shared.c

Line 362 in ed1582e

byte *nxt = instr_encode_check_reachability(dcontext, instr, buf, &has_instr_opnds);

The branch shouldn't affect state restoration at all. For state restoration purpose, maybe we'll need to somehow recognise that this branch is the syscall skip one, and then set its target to some nearby instr?

[derekbruening]

This seems to occur during the instr_get_eflags(inst, DR_QUERY_INCLUDE_ALL) call in drreg_event_restore_state, for the above-mentioned instr. The instr is part of the syscall mangling jumps, and the target looks slightly different than other branch instrs: b @0x0000fffd40a0bec0[8byte] vs b $0x0000ffff846aa22c

The encoding error itself seems because the immediate is too big for b (which can handle 28-bit offset only). private_instr_encode tries to encode the branch instr in some buf and the offset (target - buf-address) is perhaps more than 28-bit.

dynamorio/core/ir/instr_shared.c

Line 362 in ed1582e

byte *nxt = instr_encode_check_reachability(dcontext, instr, buf, &has_instr_opnds);

Not sure I understand: what is the callstack? Somebody calls instr_length() and it fails b/c of a temp buffer reachability issue? I thought that kind of thing was all handled, using a flag or param saying to ignore reachability problems for temp encodes.

[derekbruening]

Not sure I understand: what is the callstack? Somebody calls instr_length() and it fails b/c of a temp buffer reachability issue? I thought that kind of thing was all handled, using a flag or param saying to ignore reachability problems for temp encodes.

instr_encode_ignore_reachability(), that's it.

[abhinav92003]

Thanks for the review! Apologies for the many doc style issues at the end.

instr_encode_ignore_reachability(), that's it.

I see that private_instr_encode indeed uses instr_encode_ignore_reachability to work around this issue. The encoding failure log was essentially a warning that gets printed when the first attempt fails. I considered skipping the warning completely for check_reachable (6aaef52) but decided against it as encoding may fail due to some other issue too. Instead, I just added the warning to expected output pattern for AArchXX.

[derekbruening]

Ah this needs to be documented if the restore event is passed a post-mangling instrlist. Normally clients never see mangling and this may confuse someone and cause errors as they won’t see the same thing as in the bb event. I guess decoding from the cache is similar but we should make this clear.

…

On Fri, Jun 11, 2021 at 2:50 PM Abhinav Anil Sharma < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ext/drreg/drreg.c <#4944 (comment)>: > - if (is_our_spill_or_restore(drcontext, &inst, &spill, &reg, &slot, &offs)) { - LOG(drcontext, DR_LOG_ALL, 3, - "%s @" PFX " found %s to %s offs=0x%x => slot %d\n", __FUNCTION__, - prev_pc, spill ? "spill" : "restore", get_register_name(reg), offs, slot); + /* XXX: if ilist not available, fall back to some best-effort logic. */ + ASSERT(info->fragment_info.ilist != NULL, "ilist required for state restoration"); + for (inst = instrlist_first(info->fragment_info.ilist); + inst && pc < info->raw_mcontext->pc; inst = instr_get_next(inst)) { + int len = instr_length(drcontext, inst); + pc += len; + ASSERT(pc <= info->raw_mcontext->pc, "went beyond fault pc while walking ilist"); + bool app_gpr_restored_now = false; + bool app_aflags_restored_now = false; + bool app_aflags_written_to_reg_now = false; + /* Update book-keeping for gpr/aflags spill slot or aflags spill reg. */ + if (is_our_spill_or_restore(drcontext, inst, &spill, &reg, &slot, &offs)) { I believe it is impossible to be confused on x86, since an app instr that uses %gs will be mangled into a non-far memref. Unless you're looking at the pre-mangling instrlist? mangle is set to true for the ilist that is passed to the restore event: - the multiple recreate_fragment_ilist calls at https://github.com/DynamoRIO/dynamorio/blob/4b6d1ca39fc870cd6f20618f65f5d999eb80e734/core/translate.c#L1390; and - internally in recreate_selfmod_ilist too https://github.com/DynamoRIO/dynamorio/blob/4b6d1ca39fc870cd6f20618f65f5d999eb80e734/core/translate.c#L1146 Checking for AArchXX... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4944 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRVIMUA6SCSV3VBO2WYFNLTSJLGRANCNFSM46MT7WIA> .

[abhinav92003]

Ah this needs to be documented if the restore event is passed a post-mangling instrlist. Normally clients never see mangling and this may confuse someone and cause errors as they won’t see the same thing as in the bb event. I guess decoding from the cache is similar but we should make this clear.

Added the additional information in documentation -- that the reconstructed ilist may contain DR's own mangling too.

[abhinav92003]

I found another corner case -- when there are two spills from different phases that spill the native app value without any write in between them. E.g.:

<app2app spills r1 to s1>
<insertion spills r1 to s2>
<insertion restores r1 from s2>
<insertion spills r2 to s2>
<fault>

This second spill may also be from outside-drreg spills/restores like restore_app_value_to_stolen_reg

In this case, the restore state logic should remember s1 and s2 both for r1. If it remembers only s2, then it won't be able to restore r1 as s2 is overwritten later. To handle this, I simply maintain a spill_slot -> reg mapping instead of reg -> spill_slot. Added the fix and a test case too.

I think I have handled all cases. I'll give it another look and re-request review when ready.

[derekbruening]

I suspect that this PR caused a regression in drreg restoration: #4963