Skip to content

Commit

Permalink
i#3823 multi-phase drreg: Delay slot id label
Browse files Browse the repository at this point in the history 
Moves label that contains slot id for register spill/restore instrs to after the instr instead of
before. The free spill slot selection logic that makes use of these labels scans instrs *after*
the given one, so we may miss the label if it is placed before.

Fixes order of app val spill and tool val restore instrs after an instr that reads and writes a
spilled reg. This was to take into account the label which is now after the tool restore instr.

Adds test to verify restoration of reg that was reserved in multiple phases on a fault, for
X86 and AARCHXX.

Also adds AARCHXX variant of the multi-phase slot conflict test.

Adds a note to the label instrs added by drreg-test to mark instrumentation locations. This is
to avoid conflicts with other lqbel instrs.

Issue: #3823, #2985
  • Loading branch information
@abhinav92003
abhinav92003 committed May 27, 2021
1 parent 6d84fea commit 6183b7b
Show file tree
Hide file tree
Showing 4 changed files with 216 additions and 51 deletions.
57 changes: 35 additions & 22 deletions ext/drreg/drreg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ has_pending_slot_usage_by_prior_pass(per_thread_t *pt, instrlist_t *ilist, instr
return false;
for (instr_t *in = where; in != NULL; in = instr_get_next(in)) {
/* We store data about spill slot usage in the data area of a label instr
* immediately preceding the usage.
* immediately following the usage.
*/
if (instr_is_label(in) &&
instr_get_note(in) == (void *)get_drreg_note_val(DRREG_NOTE_SPILL_SLOT_ID)) {
Expand Down Expand Up @@ -254,6 +254,13 @@ spill_reg(void *drcontext, per_thread_t *pt, reg_id_t reg, uint slot, instrlist_
if (slot == AFLAGS_SLOT)
pt->aflags.ever_spilled = true;
pt->slot_use[slot] = reg;
if (slot < ops.num_spill_slots) {
dr_insert_write_raw_tls(drcontext, ilist, where, tls_seg,
tls_slot_offs + slot * sizeof(reg_t), reg);
} else {
dr_spill_slot_t DR_slot = (dr_spill_slot_t)(slot - ops.num_spill_slots);
dr_save_reg(drcontext, ilist, where, reg, DR_slot);
}
instr_t *spill_slot_data_label = INSTR_CREATE_label(drcontext);
dr_instr_label_data_t *data = instr_get_label_data_area(spill_slot_data_label);
ASSERT(data != NULL, "failed to find label's data area");
Expand All @@ -264,15 +271,8 @@ spill_reg(void *drcontext, per_thread_t *pt, reg_id_t reg, uint slot, instrlist_
}
instr_set_note(spill_slot_data_label,
(void *)get_drreg_note_val(DRREG_NOTE_SPILL_SLOT_ID));
/* This must immediately precede the spill instrs inserted below. */
/* This must immediately follow the spill instrs inserted above. */
PRE(ilist, where, spill_slot_data_label);
if (slot < ops.num_spill_slots) {
dr_insert_write_raw_tls(drcontext, ilist, where, tls_seg,
tls_slot_offs + slot * sizeof(reg_t), reg);
} else {
dr_spill_slot_t DR_slot = (dr_spill_slot_t)(slot - ops.num_spill_slots);
dr_save_reg(drcontext, ilist, where, reg, DR_slot);
}

#ifdef DEBUG
if (slot > stats_max_slot)
Expand All @@ -293,6 +293,15 @@ restore_reg(void *drcontext, per_thread_t *pt, reg_id_t reg, uint slot,
"internal tracking error");
if (release) {
pt->slot_use[slot] = DR_REG_NULL;
}
if (slot < ops.num_spill_slots) {
dr_insert_read_raw_tls(drcontext, ilist, where, tls_seg,
tls_slot_offs + slot * sizeof(reg_t), reg);
} else {
dr_spill_slot_t DR_slot = (dr_spill_slot_t)(slot - ops.num_spill_slots);
dr_restore_reg(drcontext, ilist, where, reg, DR_slot);
}
if (release) {
instr_t *spill_slot_data_label = INSTR_CREATE_label(drcontext);
dr_instr_label_data_t *data = instr_get_label_data_area(spill_slot_data_label);
ASSERT(data != NULL, "failed to find label's data area");
Expand All @@ -306,13 +315,6 @@ restore_reg(void *drcontext, per_thread_t *pt, reg_id_t reg, uint slot,
/* This must immediately precede the restore instrs inserted below. */
PRE(ilist, where, spill_slot_data_label);
}
if (slot < ops.num_spill_slots) {
dr_insert_read_raw_tls(drcontext, ilist, where, tls_seg,
tls_slot_offs + slot * sizeof(reg_t), reg);
} else {
dr_spill_slot_t DR_slot = (dr_spill_slot_t)(slot - ops.num_spill_slots);
dr_restore_reg(drcontext, ilist, where, reg, DR_slot);
}
}

static reg_t
Expand Down Expand Up @@ -700,12 +702,23 @@ drreg_event_bb_insert_late(void *drcontext, void *tag, instrlist_t *bb, instr_t
}
spill_reg(drcontext, pt, reg, tmp_slot, bb, inst);
}
spill_reg(drcontext, pt, reg, pt->reg[GPR_IDX(reg)].slot, bb,
/* If reads and writes, make sure tool-restore and app-spill
* are in the proper order.
*/
restored_for_read[GPR_IDX(reg)] ? instr_get_prev(next)
: next /*after*/);
/* If reads and writes, make sure tool-restore and app-spill
* are in the proper order.
*/
if (restored_for_read[GPR_IDX(reg)]) {
ASSERT(instr_get_prev(next) != NULL &&
instr_is_label(instr_get_prev(next)) &&
instr_get_note(instr_get_prev(next)) ==
(void *)get_drreg_note_val(DRREG_NOTE_SPILL_SLOT_ID),
"instr before 'next' should be a label with spill slot id");
ASSERT(instr_get_prev(instr_get_prev(next)) != NULL,
"missing reg restore after app read");
spill_reg(drcontext, pt, reg, pt->reg[GPR_IDX(reg)].slot, bb,
instr_get_prev(instr_get_prev(next)));
} else {
spill_reg(drcontext, pt, reg, pt->reg[GPR_IDX(reg)].slot, bb,
next /*after*/);
}
pt->reg[GPR_IDX(reg)].ever_spilled = true;
if (!restored_for_read[GPR_IDX(reg)])
restore_reg(drcontext, pt, reg, tmp_slot, bb, next /*after*/, true);
Expand Down
5 changes: 4 additions & 1 deletion suite/tests/client-interface/drreg-test-shared.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
* specific tests in the client.
* We limit to 16 bits to work on ARM.
*/
#define DRREG_TEST_CONST(num) f1f##num
#define DRREG_TEST_CONST(num) f1##num

#ifdef X86
/* Set SF,ZF,AF,PF,CF, and bit 1 is always 1 => 0xd7 */
Expand Down Expand Up @@ -127,3 +127,6 @@

#define DRREG_TEST_13_ASM MAKE_HEX_ASM(DRREG_TEST_CONST(13))
#define DRREG_TEST_13_C MAKE_HEX_C(DRREG_TEST_CONST(13))

#define DRREG_TEST_14_ASM MAKE_HEX_ASM(DRREG_TEST_CONST(14))
#define DRREG_TEST_14_C MAKE_HEX_C(DRREG_TEST_CONST(14))
115 changes: 111 additions & 4 deletions suite/tests/client-interface/drreg-test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ void
test_asm_faultD();
void
test_asm_faultE();
void
test_asm_faultF();

static SIGJMP_BUF mark;

Expand Down Expand Up @@ -111,6 +113,16 @@ handle_signal4(int signal, siginfo_t *siginfo, ucontext_t *ucxt)
# endif
SIGLONGJMP(mark, 1);
}
static void
handle_signal5(int signal, siginfo_t *siginfo, ucontext_t *ucxt)
{
if (signal == SIGILL) {
sigcontext_t *sc = SIGCXT_FROM_UCXT(ucxt);
if (sc->TEST_REG_SIG != DRREG_TEST_14_C)
print("ERROR5: spilled register value was not preserved!\n");
}
SIGLONGJMP(mark, 1);
}
# elif defined(WINDOWS)
# include <windows.h>
static LONG WINAPI
Expand Down Expand Up @@ -156,6 +168,15 @@ handle_exception4(struct _EXCEPTION_POINTERS *ep)
# endif
SIGLONGJMP(mark, 1);
}
static LONG WINAPI
handle_exception5(struct _EXCEPTION_POINTERS *ep)
{
if (ep->ExceptionRecord->ExceptionCode == EXCEPTION_ILLEGAL_INSTRUCTION) {
if (ep->ContextRecord->TEST_REG_CXT != DRREG_TEST_14_C)
print("ERROR: spilled register value was not preserved!\n");
}
SIGLONGJMP(mark, 1);
}
# endif
int
main(int argc, const char *argv[])
Expand Down Expand Up @@ -220,6 +241,17 @@ main(int argc, const char *argv[])
test_asm_faultE();
}

# if defined(UNIX)
intercept_signal(SIGILL, (handler_3_t)&handle_signal5, false);
# elif defined(WINDOWS)
SetUnhandledExceptionFilter(&handle_exception5);
# endif

/* Test fault reg restore (multi-phase) */
if (SIGSETJMP(mark) == 0) {
test_asm_faultF();
}

/* XXX i#511: add more fault tests and other tricky corner cases */

print("drreg-test finished\n");
Expand Down Expand Up @@ -314,13 +346,13 @@ GLOBAL_LABEL(FUNCNAME:)
test13:
mov TEST_REG_ASM, DRREG_TEST_13_ASM
mov TEST_REG_ASM, DRREG_TEST_13_ASM
mov REG_XAX, 123
mov REG_XCX, 456
nop
add REG_XAX, REG_XCX
jmp test13_done
test13_done:
jmp epilog
/* Fail if reg was not restored correctly. */
cmp TEST_REG_ASM, DRREG_TEST_13_ASM
jne 0
jmp epilog

epilog:
add REG_XSP, FRAME_PADDING /* make a legal SEH64 epilog */
Expand Down Expand Up @@ -352,6 +384,18 @@ GLOBAL_LABEL(FUNCNAME:)
sel TEST_REG_ASM, r0, r0
cmp TEST_REG_ASM, sp

b test13
/* Test 13: Multi-phase reg spill slot conflicts. */
test13:
mov TEST_REG_ASM, DRREG_TEST_13_ASM
mov TEST_REG_ASM, DRREG_TEST_13_ASM
nop
b test13_done
test13_done:
/* Fail if reg was not restored correctly. */
cmp TEST_REG_ASM, DRREG_TEST_13_ASM
bne 0

b epilog
epilog:
bx lr
Expand Down Expand Up @@ -381,6 +425,19 @@ GLOBAL_LABEL(FUNCNAME:)
csel TEST_REG_ASM, x0, x0, gt
cmp TEST_REG_ASM, x0

b test13
/* Test 13: Multi-phase reg spill slot conflicts. */
test13:
movz TEST_REG_ASM, DRREG_TEST_13_ASM
movz TEST_REG_ASM, DRREG_TEST_13_ASM
nop
b test13_done
test13_done:
/* Fail if reg was not restored correctly. */
movz TEST_REG2_ASM, DRREG_TEST_13_ASM
cmp TEST_REG_ASM, TEST_REG2_ASM
bne 0

b epilog
epilog:
ret
Expand Down Expand Up @@ -626,6 +683,56 @@ OB jmp test10
#endif
END_FUNC(FUNCNAME)
#undef FUNCNAME

/* Test 14: restore on fault for gpr reserved in multiple phases */
#define FUNCNAME test_asm_faultF
DECLARE_FUNC_SEH(FUNCNAME)
GLOBAL_LABEL(FUNCNAME:)
#ifdef X86
PUSH_CALLEE_SAVED_REGS()
sub REG_XSP, FRAME_PADDING /* align */
END_PROLOG

jmp test14
test14:
mov TEST_REG_ASM, DRREG_TEST_14_ASM
mov TEST_REG_ASM, DRREG_TEST_14_ASM
nop
ud2

jmp epilog14
epilog14:
add REG_XSP, FRAME_PADDING /* make a legal SEH64 epilog */
POP_CALLEE_SAVED_REGS()
ret
#elif defined(ARM)
/* XXX i#3289: prologue missing */
b test14
test14:
movw TEST_REG_ASM, DRREG_TEST_14_ASM
movw TEST_REG_ASM, DRREG_TEST_14_ASM
nop
.word 0xe7f000f0 /* udf */

b epilog14
epilog14:
bx lr
#elif defined(AARCH64)
/* XXX i#3289: prologue missing */
b test14
test14:
movz TEST_REG_ASM, DRREG_TEST_14_ASM
movz TEST_REG_ASM, DRREG_TEST_14_ASM
nop
.inst 0xf36d19 /* udf */

b epilog14
epilog14:
ret
#endif
END_FUNC(FUNCNAME)
#undef FUNCNAME

END_FILE
#endif
/* clang-format on */
Loading

0 comments on commit 6183b7b

Please sign in to comment.