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#6417: restore registers before syscall. #6475

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+125 −1
Open

i#6417: restore registers before syscall. #6475

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
925d9e6
i#6417: restore output register for syscall.
ivankyluk Nov 27, 2023
b463ae2
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 5, 2023
9bba603
Incorporate review comments.
ivankyluk Dec 5, 2023
c6cf84b
Fix CI test breakages.
ivankyluk Dec 5, 2023
185cd63
Rename x86 templatex files, and fix Aarch64 syscall number.
ivankyluk Dec 5, 2023
d0f8a79
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 5, 2023
438bca2
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 15, 2023
94073fe
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Mar 25, 2024
732be3c
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk May 13, 2024
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
22 changes: 14 additions & 8 deletions ext/drreg/drreg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,14 +385,20 @@ drreg_event_bb_analysis(void *drcontext, void *tag, instrlist_t *bb, bool for_tr
/* DRi#1849: COND_SRCS here includes addressing regs in dsts */
if (instr_reads_from_reg(inst, reg, DR_QUERY_INCLUDE_COND_SRCS))
value = REG_LIVE;
/* make sure we don't consider writes to sub-regs */
else if (instr_writes_to_exact_reg(inst, reg, DR_QUERY_INCLUDE_COND_SRCS)
/* A write to a 32-bit reg zeroes the top 32 bits for x86_64 and
* aarch64.
*/
IF_X64(||
instr_writes_to_exact_reg(inst, reg_64_to_32(reg),
DR_QUERY_INCLUDE_COND_SRCS)))
/* Make sure we don't consider writes to sub-regs. i#6417: in case
ivankyluk marked this conversation as resolved.
Show resolved Hide resolved
* of a syscall, restore the value of the output register since it
Copy link
Contributor

Choose a reason for hiding this comment

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

"restore the value": this doesn't achieve that for an inlined syscall. It avoids treating it as dead, which will end up restoring the value for a fragment-final syscall (I think the drreg end of block restores come in before the syscall instr that ends the block instead of after b/c of DR's rules for block termination). But this doesn't solve the problem for a syscall inlined into a block or trace (==superblock, not a memtrace). For that, drreg should treat the syscall as a barrier and actively restore all app values prior to it. This may be as simple as adding an instr_is_syscall check where DR_NOTE_REG_BARRIER is identified today; but then test(s) need to be created too.

If you don't want to tackle that here, this change as you have it will fix drmemtrace when -disable_traces is on (which is not the case by default: maybe it should be; it is the case for our own internal uses), so one option is to put in TODO comments that clearly explain that more is needed. A new issue should be filed I would think to cover the syscall barrier.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually a little confused as to whether a trace will be built through a non-ignorable syscall (drmemtrace marks all syscalls as non-ignorable via the filter event) -- so I'm not certain what happens without -disable_traces in drmemtrace. I would think it can't build the trace as it needs to invoke the client callbacks; but at a glance I don't see FRAG_MUST_END_TRACE being added. Probably just missed it.

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've updated the code to treat syscall as a barrier to restore register values. Please take another look,

* might be used as an input parameter for the kernel. For example,
* ECX is used as an input parameter for mmap2 for length, as well
* as the output parameter.
*/
else if (!instr_is_syscall(inst) &&
(instr_writes_to_exact_reg(inst, reg, DR_QUERY_INCLUDE_COND_SRCS)
/* A write to a 32-bit reg zeroes the top 32 bits for x86_64 and
* aarch64.
*/
IF_X64(||
instr_writes_to_exact_reg(inst, reg_64_to_32(reg),
DR_QUERY_INCLUDE_COND_SRCS))))
value = REG_DEAD;
else if (xfer)
value = REG_LIVE;
Expand Down
Loading