Call drreg_restore_all prior to clean calls

[johnfxgalea]

In order for clean calls to have access to a machine context that contains original register/flag values, we need to call drreg's drreg_restore_all(). Essentially, the issue is that due to drreg's lazy restoration, registers may not have their original values upon invoking a clean call.

drreg_restore_app_values() is not appropriate as it only works on a passed operand.

[johnfxgalea]

Xref #4120

[derekbruening]

There is a performance tradeoff here. Many clean calls do not care about the values of most or all application registers. Some care about just one. E.g., dr_insert_mbr_instrumentation() only examines the branch target, and thus drreg_restore_app_values(,, instr, instr_get_target(instr),) should be called (as modxfer.c does) rather than drreg_restore_all().

[johnfxgalea]

Indeed, the issue here is when a user callback is triggered because we cannot make hard assumptions on its functionality, e.g. drwrap.

[derekbruening]

The drwrap clean call interaction with drreg was in fact hit by a user of winafl: https://groups.google.com/g/dynamorio-users/c/ccXXk87SxNE

[derekbruening]

Ideas:

1. Have clean calls take a flag saying whether the user plans to access the
   app context (and which regs?) and they insert a label and drreg walks all
   the newly inserted instrs (which is something new: before it ignored
   inserted instrs during insertion phase) and treats that like the
   annotation label? (Maybe we make a new type of label and annot inserts
   it additionally.)

2. We add a core DR raise + event API so different components can talk to
   each other on requests for app state: so users of clean calls (or clean
   calls themselves, taking in a flag like the prior idea) post a request,
   and drreg subscribes, or sthg like that: message passing instead of
   labels.

3. Make drreg wrappers of clean call insertion calls and users have to use
   those instead of the core DR API and pass in which regs they need.

[derekbruening]

A 4th possibility:

4. Have core directly call drreg restore.
   But We'd rather keep the core separate.

For the 2nd: better to make the event just "clean call here" and not "app state needed here"
b/c the latter implies dr_save_reg(), etc. should restore here.

But we're leaning toward the first one with the label.

For that:

drreg has to walk backward; have drmgr provide a query to identify the
prior app instr (can't be delete in insertion phase so it can just keep
ptr).

New type of label and annotation inserts it too. Maybe it records
which regs are needed (update: no, it doesn't: see below).

New flag to dr_insert_clean_call: default to needs-all-regs for safety at
perf cost? No, most clean calls don't, so leave legacy at cur perf.

But generally only 1 to 3 regs spilled: so maybe little perf advantage of
restoring just 1: only saving 0-2 loads at most. So just one flag saying
whether to restore all, or none, and we can add that to dr_cleancall_save_t
without changing any signatures.

Add a comment by dr_get_mcontext() about setting this new flag.

Label should be before clean call inserts its stuff (b/c may pass app val
as opnd to clean call) rather than in middle or sthg.

---

Not yet figured out:

How apply to drwrap: does it give users the option to get app state or not?
How does this interact w/ the drwrap fast call options which IIRC provide
just a couple of registers?

[derekbruening]

Re: the drwrap questions: it looks like DRWRAP_FAST_CLEANCALLS is still using a dr_insert_clean_call (I thought it wasn't...maybe thinking of some past DrMemory optimizations) and it is just avoiding saving flags and some SIMD values: so I think it would still work w/ the label proposal.

Re: drwrap providing an option: getting arg or return values does need the barrier so it seems the common case would want it; but since drwrap is commonly used and performance is an issue maybe it should provide an option to not restore app values for cases where arg or return values are not needed.

[johnfxgalea]

I really like the simplicity of option 3 but it does not transcend well across other DR extensions; drwrap would need to be aware of drreg and the new wrappers (at the moment, drreg is not included in drwrap). Yeah... I would lean more towards idea 1 as it appears to be less intrusive.

[derekbruening]

I think we need the counterpart for dr_set_mcontext() called in a clean call: we need a label requesting that drreg update the spilled app values.

[derekbruening]

@johnfxgalea I'm updating instances of dr_insert_clean_call that call dr_{s,g}et_mcontext and there are some in drbbdup.c -- but they're generated in a cache and I'm not sure whether they need the new DR_CLEANCALL_READS_APP_CONTEXT | DR_CLEANCALL_WRITES_APP_CONTEXT?

[johnfxgalea]

@derekbruening Thanks for informing me.

There are two places where drbbdup uses clean calls. The first invokes drbbdup_inc_bail_count.

dynamorio/ext/drbbdup/drbbdup.c

Line 817 in b8f8730

static void

This function does not use mcontext, so no change needed here.

The second use of clean call insertion happens inside a cache:

dynamorio/ext/drbbdup/drbbdup.c

Line 1315 in b8f8730

dr_insert_clean_call(drcontext, ilist, NULL, (void *)clean_call_func, false, 0);

In particular, the call targets drbbdup_handle_new_case, which in turn gets and modifies the machine context for redirection (via dr_redirect_execution).

dynamorio/ext/drbbdup/drbbdup.c

Line 1208 in b8f8730

drbbdup_handle_new_case()

The function requires that a register, which is used as scratch at the time of the function's call, contains drbbdup information. In other words, the function assumes that this GPR register and the aflags are the only registers that do not contain original values. Prior to redirection, the function calls a helper, namely drbbdup_prepare_redirect, to restore these registers back to their original values. Naturally, this involves modifying the mcontext.

dynamorio/ext/drbbdup/drbbdup.c

Line 1188 in b8f8730

static void

[derekbruening]

The function requires that a register, which is used as scratch at the time of the function's call, contains drbbdup information. In other words, the function assumes that this GPR register and the aflags are the only registers that do not contain original values. Prior to redirection, the function calls a helper, namely drbbdup_prepare_redirect, to restore these registers back to their original values. Naturally, this involves modifying the mcontext.

So it sounds like these new clean call flags are not needed for drbbdup and would in fact break the current model there.

[derekbruening]

We picked idea 1 above, but it doesn't work: drreg can't walk backward as it relies on forward time and can't move its state machine backward to the point of the label.

It also has the problem of the temp move of the tool value and restore after being in the wrong place: after the label is still before the call! We'd need a 3rd label after the app-write label after the call to restore tool values if nec (and so need new drreg state too?)

This problem should go away if we call drreg at the time we insert the clean call (may also need a new label there for the right "after" point): so it's unique to the label soln.

The new plan is to go with idea 2 where DR has a clean call insertion event and drreg registers for it.

How handle annotations?

Can't call cleancall cb at annot label insert time: has to be at drmgr
insert phase.
So either:
A) drmgr wraps DR's cleancall cb, looks for DR_NOTE_ANNOTATION, and invokes
cleancall cb there w/ DR_CLEANCALL_READS_APP_CONTEXT
B) drreg looks directly for DR_NOTE_ANNOTATION like in HEAD today

Move annot clean call into client that inserts in insertion phase so it
doesn't need special handling?
Could study moving annot instr pattern from DR interp inner loop into an
app2app too.
=> #5160 feature request for future work

Fits better w/ clean call callback and removes drreg looking for
DR_NOTE_ANNOTATION. Might help instru2instru or other places b/c clean
call would be visible.

If we collapsed drmgr into core DR: I would suggest moving the clean call,
still done by DR, to insertion phase, for same benefits

What ab clean calls in other phases?

• Just don't support in app2app (should insert label and then insert clean
  call in insertion phase)
• Ignore for non-insertion

[derekbruening]

This gets more and more complicated: for control flow that can skip a clean call or not, I added DR_CLEANCALL_MULTIPATH which makes DR_CLEANCALL_READS_APP_CONTEXT invoke drreg_statelessly_restore_app_value() for flags + all GPRs.

But that clobbers tool values: so I'm just documenting that and saying you have to do it manually if that's no good.

And then: what about for DR_CLEANCALL_WRITES_APP_CONTEXT | DR_CLEANCALL_MULTIPATH? Go try to write a stateless version of that (again disallowing live tool values)? Or don't support that flag combo? I'm not supporting it.