Skip to content
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

Add a mechanism for clients to store metadata for no-recreate translation #3801

Open
derekbruening opened this issue Aug 27, 2019 · 7 comments

Comments

@derekbruening
Copy link
Contributor

Today, clients have to decode from cache instructions to deduce how to
restore state (combined with heap-stored metadata they may have kept). The
DR core, however, uses instrlist recreation metadata as part of its state
updates for mangling expansions, but does not make it easy for clients to
do the same. This issue covers trying to add a convenience mechanism to
pass data from the bb event w/ translating=true to the restore_state event.

@hgreving2304
Copy link

xref #2985 (a case where this will be needed)

@hgreving2304
Copy link

Main problem seems to be that for pending-deletion fragments, there is no ilist. So just adding a new restore_event callback that gets the ilist isn't enough. Would we need a restore_event_from_ilist AND restore_event_from_info? Is bb_event even called in this case?

@hgreving2304
Copy link

Also DR_EMIT_STORE_TRANSLATIONS.

@derekbruening
Copy link
Contributor Author

Yes, to be like the core, clients would need to handle recreation from a list and restoring from stored info, so this is not all that convenient. Lower priority.

abhinav92003 added a commit that referenced this issue Jun 14, 2021
Revamps the drreg state restoration logic in drreg_event_restore_state to
correctly handle various corner cases like multiple restores (i#4939), multiple
spills, aflags in reg (i#4933), multi-phase use (i#3823) leading to nested
or otherwise overlapping spill regions, and app instrs that look like drreg
spill/restore instrs.

Makes the reconstructed ilist available to drreg_event_restore_state (i#3801).
The additional metadata that is used by the new logic is only whether an instr
is an app or meta instr. For cases when the reconstructed ilist is not
available, we continue to use the old best-effort logic. Highlighted its caveats
in comments.

The new logic looks at the complete ilist to track the movement of the app value
of gprs/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 or if there's an
app instr that writes to the spilled gpr/aflags.

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 gpr's/aflags' spill slot. Restores from spill slots
also install a native value.

Adds tests for multi-phase gpr reservation cases (nested/overlapping spill
regions), multiple spills for gpr and multiple restores for gpr. Tests for these
scenarios for aflags will be added when multi-phase drreg support is added for
aflags.

Tracks movement of aflags to registers more robustly to fix i#4933. Also adds a
test for this case.

Avoids confusing app instrs that write using the stolen reg on AArch64 with
drreg spill or restore instrs. Also adds a test that verifies this fix.

Adds documentation about faulting fragment's reconstructed ilist being available
in restore state event callbacks.

Issue: #4933, #4939, #3823, #3801
Fixes: #4933
Fixes: #4939
@abhinav92003
Copy link
Contributor

Another use case for passing metadata to state restoration:

The current drreg state restore algorithm does not restore the app value of a gpr/aflags if they are dead at the fault instruction. This aligns with general drreg use. However, drreg should restore even dead gprs/aflags when drreg_options_t.conservative is set. It is difficult to achieve this without additional metadata to guide state restoration.

@derekbruening
Copy link
Contributor Author

The original description is a little confusing above. Let's sharpen the focus here on the
no-instrlist case.

Also xref #5005 on drx's scatter-gather translation decode-only
approach being fragile; drreg's without an
instrlist is also fragile; drbbdup today bails (we'll have to remedy that for
b/254312104,i#5686 but it again will be fragile): without metadata from labels
in the recreated instrlist, clients are faced with extra complexity of decoding
and using heuristics to guess at their own instrumentation.

This issue is about improving the situation by having someone store ilist labels
for no-ilist fragments, just like DR stores pc ranges.

As an aside: note that we could not recreate even if we wanted to for cases
where clients have non-idempotent instrumentation; so even if we tried to
recreate for pending-delete fragments by making a code copy similar to our
selfmod copy we should still have to handle the no-recreate case.

Maybe we can assume that a small % of fragments are non-recreate, though we
should try to keep the storage cost for keeping metadata low. Maybe all we need
is a list of PC,label_value pairs.

Is the burden on the client for storing that? Xref complexities of per-bb data
vs bb deletion and tag re-use (esp for pending-delete). Maybe DR should provide
a user_data pointer (and drmgr multi-plexes or sthg) per fragment? Or DR
provides a stored translation-specific user_data.

@derekbruening derekbruening changed the title Add a mechanism for clients to pass metadata from the bb event translating=true to the restore_state event Add a mechanism for clients to store metadata for no-recreate translation Jan 19, 2023
@derekbruening
Copy link
Contributor Author

drbbdup is a good test case here. drbbdup today bails on no-ilist: it does not implement decoding and heuristics to find the bbdup copies. Can easily trigger failure in the new drx-scattergather-bbdup test (for #5686) by having it return DR_EMIT_STORE_TRANSLATIONS.

derekbruening added a commit that referenced this issue Jan 19, 2023
Moves the drbbdup restore state event to a very early priority and has
it modify the restore state parameters to focus on the containing copy
of the block where the restore resides.  It modifies the instruction
list and the cache start pc.

Adds a new test drx-scattergather-bbdup by adding drbbdup to the
existing drx-scattergather test.  This test fails to restore without
the fix described above, due to the scatter/gather expansion restore
state machine getting confused across multiple copies.

Does not augment drbbdup's restore to handle the no-ilist case: leaves
that for the general #3801 mechanism.

Issue: #5686
derekbruening added a commit that referenced this issue Jan 20, 2023
Moves the drbbdup restore state event to a very early priority and has
it modify the restore state parameters to focus on the containing copy
of the block where the restore resides.  It modifies the instruction
list and the cache start pc.

Adds a new test drx-scattergather-bbdup by adding drbbdup to the
existing drx-scattergather test.  This test fails to restore without
the fix described above, due to the scatter/gather expansion restore
state machine getting confused across multiple copies.

Does not augment drbbdup's restore to handle the no-ilist case: leaves
that for the general #3801 mechanism.

Fixes an uninitialized var compiler warning in the aarch64 codec change
PR #5821 #3044 b63dbda.

Issue: #5686
dolanzhao pushed a commit that referenced this issue Jan 30, 2023
Moves the drbbdup restore state event to a very early priority and has
it modify the restore state parameters to focus on the containing copy
of the block where the restore resides.  It modifies the instruction
list and the cache start pc.

Adds a new test drx-scattergather-bbdup by adding drbbdup to the
existing drx-scattergather test.  This test fails to restore without
the fix described above, due to the scatter/gather expansion restore
state machine getting confused across multiple copies.

Does not augment drbbdup's restore to handle the no-ilist case: leaves
that for the general #3801 mechanism.

Fixes an uninitialized var compiler warning in the aarch64 codec change
PR #5821 #3044 b63dbda.

Issue: #5686
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants