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.

Sign up for GitHub

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

i#4687: conservatively handle aflags for unrecognized (OP_xx) instructions on AArch64 #4688

Merged
merged 9 commits into from
Jan 25, 2021
Merged

i#4687: conservatively handle aflags for unrecognized (OP_xx) instructions on AArch64 #4688

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fd040d9
i#4687: handle OP_xx insts when using drreg_unreserve_aflags on AArch64
sapostolakis Jan 23, 2021
b75aea8
OP_xx only applies to AArch64
sapostolakis Jan 23, 2021
385f7a4
Clang-format
sapostolakis Jan 23, 2021
23c6225
Link workaround with issue for permanent fix
sapostolakis Jan 23, 2021
a5e4073
Add aflags as read and written to every OP_xx
sapostolakis Jan 25, 2021
7523a1a
Address review comments
sapostolakis Jan 25, 2021
9b43b13
Merge branch 'master' into i4687-drreg-restore-aflags
sapostolakis Jan 25, 2021
e8b4102
fix typo in comments
sapostolakis Jan 25, 2021
0d2f303
Merge branch 'master' into i4687-drreg-restore-aflags
sapostolakis Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ext/drreg/drreg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,12 @@ drreg_insert_restore_all(void *drcontext, instrlist_t *bb, instr_t *inst,
uint aflags = (uint)(ptr_uint_t)drvector_get_entry(&pt->aflags.live, pt->live_idx);
if (!pt->aflags.native &&
(force_restore ||
TESTANY(EFLAGS_READ_ARITH, instr_get_eflags(inst, DR_QUERY_DEFAULT)) ||
/* On AArch64, unrecognized instructions are decoded as instances of a generic
* instruction, OP_xx. For these instructions conservatively restore aflags in
* case they read/write the aflags.
*/
sapostolakis marked this conversation as resolved.
Show resolved Hide resolved
IF_AARCH64(instr_get_opcode(inst) == OP_xx ||)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that all of the missing opcodes are vector instructions, I would expect a very small fraction to read or write flags (or GPR's for that matter). I wonder if adding the couple of opcodes that affect flags and GPR's would not be much work. Though it's certainly more work than this workaround. Let's wait for advice from those more familiar with the a64 decoder: @AssadHashmi?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that all of the missing opcodes are vector instructions, I would expect a very small fraction to read or write flags (or GPR's for that matter). I wonder if adding the couple of opcodes that affect flags and GPR's would not be much work. Though it's certainly more work than this workaround. Let's wait for advice from those more familiar with the a64 decoder: @AssadHashmi?

The resourcing planned to address AArch64 decoder gaps has been diverted elsewhere at the present time. Reluctantly, I'd advise carrying on with the workaround. This is far from ideal for AArch64 support but we currently have to work within limited constraints. Hopefully the situation will change later in the year.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about the implications here: workarounds will have to added to multiple places and not just drreg. The drcachesim tracer does GPR analysis to elide same-base addresses. The memval tool looks at register usage. Dr. Memory, being ported now, looks at everything. (The core does flags analysis for fragment prefixes and stay-on-trace comparisons -- though I believe these have no problem today due to flags-less IBL and no trace support yet for AArch64.)

@AssadHashmi is it the case that OP_xx insructions have GPR operands in place as hinted at here #2626 (comment)? Does that reliably cover all GPR usage in such instructions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AssadHashmi is it the case that OP_xx instructions have GPR operands in place as hinted at here #2626 (comment)?

@derekbruening yes I believe that is the case. @egrimley can confirm.

Does that reliably cover all GPR usage in such instructions?

The majority, if not all, OP_xx instructions are SIMD. Of those, GPR usage is restricted to an index register. Based on that you should be reliably covered. There may be cases of non-SIMD OP_xx but even then you should be ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to summarize, the recommendation is:

  • Rely on OP_xx decoding marking all possible GPR register usage as both reads and writes, which forces restores, prevents assuming a GPR is dead, voids drcachesim elision, etc. This eliminates correctness concerns, but at a performance cost.
  • Change all library and client code that looks at aflags to treat OP_xx as reading and writing them.

I would then propose: let's tweak the decoder to make the 2nd point automatic: to add aflags as read and written to every OP_xx. That might be a very simple change that would then remove the need to find and change any other code and put all the workarounds in a central place. Then we're covered for correctness for flags and GPR's and only SIMD registers have correctness issues; flags and GPR's only have performance issues.

TESTANY(EFLAGS_READ_ARITH, instr_get_eflags(inst, DR_QUERY_DEFAULT)) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only handling reading flags: there are also points where drreg needs to update a spilled aflags value after an app instruction writes to the flags. That point would need a workaround as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this workaround handles writing flags as well. My understanding is that even if an unrecognized application instruction (xx) writes to the flags, this workaround will force the flags to be restored before xx, effectively preventing insertion of a restore after xx that overwrites the write to the flags by xx. If there is another save/restore pair by drreg after the 'xx' inst, it will not change the application view of the flags. Are you thinking of something else or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restoring is not unreserving. drreg will assume that the app's flags value is still stored in TLS after an app read but not write of the flags, which will be incorrect after xx writes to the flags and changes that value. drreg will proceed to use the wrong value when restoring on future reads or unreserves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

/* Writing just a subset needs to combine with the original unwritten */
(TESTANY(EFLAGS_WRITE_ARITH, instr_get_eflags(inst, DR_QUERY_INCLUDE_ALL)) &&
aflags != 0 /*0 means everything is dead*/) ||
Expand Down