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#2210 clean call: Implement parameter preparation on ARM and AArch64. #2260

Merged
merged 6 commits into from
Mar 5, 2017
Merged

i#2210 clean call: Implement parameter preparation on ARM and AArch64. #2260

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0efa381
i#2210 clean call: Implement parameter preparation on ARM and AArch64.
egrimley-arm Mar 2, 2017
210d5ff
i#2210 clean call: Implement parameter preparation on ARM and AArch64.
egrimley-arm Mar 3, 2017
9f4193f
Add FIXME.
egrimley-arm Mar 3, 2017
e160b1f
Merge branch 'master' into i2210-aarchxx-clean-call
egrimley-arm Mar 3, 2017
4b4f331
Merge branch 'master' into i2210-aarchxx-clean-call
egrimley-arm Mar 4, 2017
c99c52f
Merge branch 'master' into i2210-aarchxx-clean-call
egrimley-arm Mar 5, 2017
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
257 changes: 221 additions & 36 deletions core/arch/aarchxx/mangle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,50 +577,235 @@ shrink_reg_for_param(reg_id_t regular, opnd_t arg)
}
#endif /* !AARCH64 */

/* Return true if opnd is a register, but not XSP, or immediate zero on AArch64. */
static bool
opnd_is_reglike(opnd_t opnd)
{
return ((opnd_is_reg(opnd) && opnd_get_reg(opnd) != DR_REG_XSP)
IF_X64(|| (opnd_is_immed_int(opnd) && opnd_get_immed_int(opnd) == 0)));
}

uint
insert_parameter_preparation(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr,
bool clean_call, uint num_args, opnd_t *args)
{
uint i;
instr_t *mark = INSTR_CREATE_label(dcontext);
PRE(ilist, instr, mark);

ASSERT(num_args == 0 || args != NULL);
/* FIXME i#1551, i#1569: we only support limited number of args for now. */
ASSERT_NOT_IMPLEMENTED(num_args <= NUM_REGPARM);
uint num_regs = num_args < NUM_REGPARM ? num_args : NUM_REGPARM;
signed char regs[NUM_REGPARM];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to figure out what type this holds: please add a comment. At first it seemed like it should be reg_id_t but it's using some custom encoding.

int usecount[NUM_REGPARM];
ptr_int_t stack_inc = 0;
uint i, j;

/* We expect every arg to be an immediate integer, a full-size register,
* or a simple memory reference (NYI).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have i# and XXX or FIXME

*/
for (i = 0; i < num_args; i++) {
if (opnd_is_immed_int(args[i])) {
insert_mov_immed_ptrsz(dcontext, opnd_get_immed_int(args[i]),
opnd_create_reg(regparms[i]),
ilist, instr_get_next(mark), NULL, NULL);
} else if (opnd_is_reg(args[i])) {
opnd_t arg = opnd_create_reg(reg_to_pointer_sized(opnd_get_reg(args[i])));
if (opnd_get_reg(arg) == DR_REG_XSP) {
instr_t *loc = instr_get_next(mark);
PRE(ilist, loc, instr_create_save_to_tls
(dcontext, regparms[i], TLS_REG0_SLOT));
insert_get_mcontext_base(dcontext, ilist, loc, regparms[i]);
PRE(ilist, loc, instr_create_restore_from_dc_via_reg
(dcontext, regparms[i], regparms[i], XSP_OFFSET));
} else if (opnd_get_reg(arg) != regparms[i]) {
POST(ilist, mark, XINST_CREATE_move(dcontext,
opnd_create_reg(regparms[i]),
arg));
CLIENT_ASSERT(opnd_is_immed_int((args[i])) ||
(opnd_is_reg(args[i]) &&
reg_get_size(opnd_get_reg(args[i])) == OPSZ_PTR) ||
opnd_is_base_disp(args[i]),
"insert_parameter_preparation: bad argument type");
ASSERT_NOT_IMPLEMENTED(!opnd_is_base_disp(args[i]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to have XXX or FIXME with i#

}

/* The strategy here is to first set up the arguments that can be set up
* without using a temporary register: stack arguments that are registers and
* register arguments that are not involved in a cycle. When this has been done,
* the value in the link register (LR) will be dead, so we can use LR as a
* temporary for setting up the remaining arguments.
*/

/* Set up stack arguments that are registers (not SP) or zero (on AArch64). */
if (num_args > NUM_REGPARM) {
uint n = num_args - NUM_REGPARM;
/* On both ARM and AArch64 the stack pointer is kept (2 * XSP_SZ)-aligned. */
stack_inc = ALIGN_FORWARD(n, 2) * XSP_SZ;
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to think for a bit as why this is being done for both arches -- a comment is in order explaining the stack alignment.

#ifdef AARCH64
for (i = 0; i < n; i += 2) {
opnd_t *arg0 = &args[NUM_REGPARM + i];
opnd_t *arg1 = i + 1 < n ? &args[NUM_REGPARM + i + 1] : NULL;
if (i == 0) {
if (i + 1 < n && opnd_is_reglike(*arg1)) {
/* stp x(...), x(...), [sp, #-(stack_inc)]! */
PRE(ilist, instr, instr_create_2dst_4src
(dcontext, OP_stp,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
-stack_inc, OPSZ_16),
opnd_create_reg(DR_REG_XSP),
opnd_is_reg(*arg0) ? *arg0 : opnd_create_reg(DR_REG_XZR),
opnd_is_reg(*arg1) ? *arg1 : opnd_create_reg(DR_REG_XZR),
opnd_create_reg(DR_REG_XSP),
opnd_create_immed_int(-stack_inc, OPSZ_PTR)));
} else if (opnd_is_reglike(*arg0)) {
/* str x(...), [sp, #-(stack_inc)]! */
PRE(ilist, instr, instr_create_2dst_3src
(dcontext, OP_str,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
-stack_inc, OPSZ_PTR),
opnd_create_reg(DR_REG_XSP),
opnd_is_reg(*arg0) ? *arg0 : opnd_create_reg(DR_REG_XZR),
opnd_create_reg(DR_REG_XSP),
opnd_create_immed_int(-stack_inc, OPSZ_PTR)));
} else {
/* sub sp, sp, #(stack_inc) */
PRE(ilist, instr, INSTR_CREATE_sub
(dcontext, opnd_create_reg(DR_REG_XSP),
opnd_create_reg(DR_REG_XSP), OPND_CREATE_INT32(stack_inc)));
}
} else if (opnd_is_reglike(*arg0)) {
if (i + 1 < n && opnd_is_reglike(*arg1)) {
/* stp x(...), x(...), [sp, #(i * XSP_SZ)] */
PRE(ilist, instr, instr_create_1dst_2src
(dcontext, OP_stp,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
i * XSP_SZ, OPSZ_16),
opnd_is_reg(*arg0) ? *arg0 : opnd_create_reg(DR_REG_XZR),
opnd_is_reg(*arg1) ? *arg1 : opnd_create_reg(DR_REG_XZR)));
} else {
/* str x(...), [sp, #(i * XSP_SZ)] */
PRE(ilist, instr, instr_create_1dst_1src
(dcontext, OP_str,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
i * XSP_SZ, OPSZ_PTR),
opnd_is_reg(*arg0) ? *arg0 : opnd_create_reg(DR_REG_XZR)));
}
} else if (i + 1 < n && opnd_is_reglike(*arg1)) {
/* str x(...), [sp, #((i + 1) * XSP_SZ)] */
PRE(ilist, instr, instr_create_1dst_1src
(dcontext, OP_str,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
(i + 1) * XSP_SZ, OPSZ_PTR),
opnd_is_reg(*arg1) ? *arg1 : opnd_create_reg(DR_REG_XZR)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the immeds and xsp?

Update: they are down below: but I don't understand why? IMHO it would be much clearer to insert them here rather than splitting up where these stack params are written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no guarantee of there being a free register at this point. See the comment: /* From now on it is safe to use LR as a temporary. */

} else {
/* FIXME i#1551, i#1569: we only implement naive parameter preparation,
* where args are all regs or immeds and do not conflict with param regs.
*/
ASSERT_NOT_IMPLEMENTED(false);
}
DODEBUG({
uint j;
/* check no reg used by arg conflicts with regparms */
for (j = 0; j < i; j++)
ASSERT_NOT_IMPLEMENTED(!opnd_uses_reg(args[j], regparms[i]));
});
#else /* ARM */
/* XXX: We could use OP_stm here, but with lots of awkward corner cases. */
PRE(ilist, instr, INSTR_CREATE_sub(dcontext, opnd_create_reg(DR_REG_XSP),
opnd_create_reg(DR_REG_XSP),
OPND_CREATE_INT32(stack_inc)));
for (i = 0; i < n; i++) {
opnd_t arg = args[NUM_REGPARM + i];
if (opnd_is_reglike(arg)) {
/* str r(...), [sp, #(i * XSP_SZ)] */
PRE(ilist, instr, XINST_CREATE_store
(dcontext, opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
i * XSP_SZ, OPSZ_PTR), arg));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto: confusing to split up where the stack params are stored: why have the reg params in between?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do the reg params in order to make sure that LR is available as a scratch register.

}
#endif
}
return 0;

/* Initialise regs[], which encodes the contents of parameter registers.
* A non-negative value x means regparms[x];
* -1 means an immediate integer;
* -2 means a non-parameter register.
*/
for (i = 0; i < num_regs; i++) {
if (opnd_is_immed_int(args[i]))
regs[i] = -1;
else {
reg_id_t reg = opnd_get_reg(args[i]);
regs[i] = -2;
Copy link
Contributor

Choose a reason for hiding this comment

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

See above too: this -1, -2 too stuff needs comments explaining, or, better, to use an enum type or something

for (j = 0; j < NUM_REGPARM; j++) {
if (reg == regparms[j]) {
regs[i] = j;
break;
}
}
}
}

/* Initialise usecount[]: how many other registers use the value in a reg. */
for (i = 0; i < num_regs; i++)
usecount[i] = 0;
for (i = 0; i < num_regs; i++) {
if (regs[i] >= 0 && regs[i] != i)
++usecount[regs[i]];
}

/* Set up register arguments that are not part of a cycle. */
{
bool changed;
do {
changed = false;
for (i = 0; i < num_regs; i++) {
if (regs[i] == i || usecount[i] != 0)
continue;
if (regs[i] == -1) {
insert_mov_immed_ptrsz(dcontext, opnd_get_immed_int(args[i]),
opnd_create_reg(regparms[i]),
ilist, instr, NULL, NULL);
} else if (regs[i] == -2 && opnd_get_reg(args[i]) == DR_REG_XSP) {
/* XXX: We could record which register has been set to the SP to
* avoid repeating this load if several arguments are set to SP.
*/
insert_get_mcontext_base(dcontext, ilist, instr, regparms[i]);
PRE(ilist, instr, instr_create_restore_from_dc_via_reg
(dcontext, regparms[i], regparms[i], XSP_OFFSET));
} else {
PRE(ilist, instr, XINST_CREATE_move(dcontext,
opnd_create_reg(regparms[i]),
args[i]));
if (regs[i] != -2)
--usecount[regs[i]];
}
regs[i] = i;
changed = true;
}
} while (changed);
}

/* From now on it is safe to use LR as a temporary. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe more explanation would make this clearer (and asserts?): I assume the logic is that the only thing to worry about is LR as a param, and since it's not a calling conv arg reg, it would have be moved into one by the loop above.


/* Set up register arguments that are in cycles. A rotation of n values is
* realised with (n + 1) moves.
*/
for (;;) {
int first, tmp;
for (i = 0; i < num_regs; i++) {
if (regs[i] != i)
break;
}
if (i >= num_regs)
break;
first = i;
PRE(ilist, instr, XINST_CREATE_move(dcontext, opnd_create_reg(DR_REG_LR),
opnd_create_reg(regparms[i])));
do {
tmp = regs[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

OK I think I understand what it's doing, shifting one into LR and everything else one down, then repeat, but comments on the approach would help, and on whether there might be a more efficient method but it would be much more complex and not worthwhile, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A rotation of n registers is implemented with (n + 1) moves in the obvious way. I don't think there's a better way.

ASSERT(0 <= tmp && tmp < num_regs);
PRE(ilist, instr,
XINST_CREATE_move(dcontext, opnd_create_reg(regparms[i]),
tmp == first ? opnd_create_reg(DR_REG_LR) :
opnd_create_reg(regparms[tmp])));
regs[i] = i;
i = tmp;
} while (tmp != first);
}

/* Set up stack arguments that are (non-zero) constants or SP. */
Copy link
Contributor

Choose a reason for hiding this comment

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

See above: IMHO these should be up by the other stack args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No guarantee of a free register earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

A comment should explain this: it was not obvious.

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 added a comment near the start of the function: /* The strategy here is to first set up ... (Yes, I do split infinitives!)

for (i = NUM_REGPARM; i < num_args; i++) {
uint off = (i - NUM_REGPARM) * XSP_SZ;
opnd_t arg = args[i];
if (!opnd_is_reglike(arg)) {
if (opnd_is_reg(arg)) {
ASSERT(opnd_get_reg(arg) == DR_REG_XSP);
insert_get_mcontext_base(dcontext, ilist, instr, DR_REG_LR);
PRE(ilist, instr, instr_create_restore_from_dc_via_reg
(dcontext, DR_REG_LR, DR_REG_LR, XSP_OFFSET));
} else {
ASSERT(opnd_is_immed_int(arg));
insert_mov_immed_ptrsz(dcontext, opnd_get_immed_int(arg),
opnd_create_reg(DR_REG_LR),
ilist, instr, NULL, NULL);
}
PRE(ilist, instr, XINST_CREATE_store
(dcontext,
opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0, off, OPSZ_PTR),
opnd_create_reg(DR_REG_LR)));
}
}

return (uint)stack_inc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason it can't be declared as a uint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might involve a lot of casts to keep Microsoft compilers happy. Though Microsoft compilers won't be looking at this code in the short term...

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 like it to be a signed type locally because I pass -stack_inc to functions that take an int. Perhaps the return type should be be ptr_int_t or ptr_uint_t: it's an increment to a pointer type and in theory one could have 2^32 arguments on a 64-bit system.

}

bool
Expand Down
11 changes: 2 additions & 9 deletions core/arch/mangle_shared.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,15 +464,8 @@ insert_meta_call_vargs(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr,
/* XXX PR 245936: let user decide whether to clean up?
* i.e., support calling a stdcall routine?
*/
#ifdef X86
PRE(ilist, instr,
INSTR_CREATE_lea(dcontext, opnd_create_reg(REG_XSP),
opnd_create_base_disp(REG_XSP, REG_NULL, 0,
stack_for_params, OPSZ_lea)));
#elif defined(ARM)
/* FIXME i#1551: NYI on ARM */
ASSERT_NOT_IMPLEMENTED(false);
#endif
PRE(ilist, instr, XINST_CREATE_add(dcontext, opnd_create_reg(REG_XSP),
OPND_CREATE_INT32(stack_for_params)));
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove all the ifdefs and use XINST_CREATE_add(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably. I don't understand why the X86 code is done how it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean why it's OP_lea? Because OP_add clobbers the flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But XINST_CREATE_add() will generate an OP_lea on X86?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please look at the docs for XINST_CREATE_add versus XINST_CREATE_add_s and the implementations. We deliberately set it up that way after some discussion.

}

#ifdef CLIENT_INTERFACE
Expand Down
Loading