i#6417: restore registers before syscall. #6475
Conversation
ext/drreg/drreg.c
Outdated
| 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 | ||
| * of a syscall, restore the value of the output register since it |
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've updated the code to treat syscall as a barrier to restore register values. Please take another look,
ivankyluk
changed the title
| * DAMAGE. | ||
| */ | ||
|
|
||
| #include <sys/mman.h> |
There was a problem hiding this comment.
Maybe this file should be in suite/tests/linux/ as mmap is unix-specific (that's the directory that has the mmap.c test as well).
| { | ||
| void *p; | ||
| const size_t size = 0x00008765; | ||
| printf("Calling mmap(0, 0x%x, 0x%x, 0x%x, -1, 0)\n", (unsigned int)size, |
There was a problem hiding this comment.
Generally we include tools.h and use print for reliable interleaving with client/DR output. Ditto below.
| * they may be used as input parameters for the kernel. For example, | ||
| * ECX is used to store the length for mmap2. Register used to store the | ||
| * output of the syscall is marked as not spilled, so we need to check for | ||
| * syscall as well. |
There was a problem hiding this comment.
But if drreg treated ecx as dead, it wouldn't have preserved the original app value, so it won't have that value to restore. (So why did the tests pass? Did drmemtrace happen to not clobber ecx b/c it picked other scratch regs?). I think we want to keep your original change to not treat a syscall dest reg as dead, and then we'll spill it and it will be available for restores. (And we won't then want this check below for instr_is_syscall).
| @@ -486,6 +486,11 @@ drreg_insert_restore_all(void *drcontext, instrlist_t *bb, instr_t *inst, | |||
| (instr_is_label(inst) && | |||
| ((ptr_uint_t)instr_get_note(inst) == DR_NOTE_ANNOTATION || | |||
| (ptr_uint_t)instr_get_note(inst) == DR_NOTE_REG_BARRIER)) || | |||
| /* i#6417: in case of a syscall, restore the values of registers since | |||
| * they may be used as input parameters for the kernel. For example, | |||
| * ECX is used to store the length for mmap2. | |||
There was a problem hiding this comment.
Hmm, this code is restoring the flags. I don't think they are an input to any syscall on any platform (they are an output on Mac).
| * they may be used as input parameters for the kernel. For example, | ||
| * ECX is used to store the length for mmap2. | ||
| */ | ||
| instr_is_syscall(inst) || |
There was a problem hiding this comment.
This looks good for pre-syscall. But after the syscall we need to update the spilled values with whatever the kernel wrote: so we need to also check for instr_is_syscall in the loop labeled /* After each app write, update spilled app values: */. Otherwise, a restore at the end of the block will bring in the old pre-syscall value.
To test, add an asm sequence to drreg-test (ifdef LINUX I guess) with a single block with a syscall in the middle and a pattern the test client can find (see existing DRREG_TEST_38_ASM e.g.) and the test client for that block can then reserve eax and ecx and clobber both register both before and after the syscall.
Verify the mmap succeeded and that the app's eax is correctly restored at the end of the block (maybe have the app check the syscall return value in the next block or sthg).
Restore the value of the output register for syscall before calling syscall. The output register might be used as an input parameter for the kernel. For example, ECX is used to store the length for syscall mmap2, as well as being used to store the output of the syscall.
The problem can be reproduced by running
bin32/drrun -debug -loglevel 4 -logdir . -stderr_mask 0xC -dumpcore_mask 0 -code_api -t drcachesim -ipc_name suite/tests/drtestpipe_miss_analyzer -simulator_type miss_analyzer -miss_count_threshold 5000 -miss_frac_threshold 0.25 -- suite/tests/bin/stride_benchmark
on AMD 32 bit system.
Add a new test syscall-mmap and use -record_syscall to verify the parameters to mmap/mmap2 are correct.
Issue: #6417