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

egrimley · 2017-03-02T11:44:04Z

The implementation should efficiently handle any (reasonable) number
of arguments that are immediate integers or full-size registers.
Test cases are added in cleancallparams.dll.c.

Fixes #2210

The implementation should efficiently handle any (reasonable) number of arguments that are immediate integers or full-size registers. Test cases are added in cleancallparams.dll.c. Fixes #2210

derekbruening

The code seems good in general, mostly I have requests for comments, some code movement, etc.

derekbruening · 2017-03-02T15:28:13Z

core/arch/aarchxx/mangle.c

+{
+    return ((opnd_is_reg(opnd) && opnd_get_reg(opnd) != DR_REG_XSP) ||
+            IF_X64_ELSE((opnd_is_immed_int(opnd) && opnd_get_immed_int(opnd) == 0),
+                        false));


Code would be easier to read as IF_X64(|| ...) rather than having an ||false

There's no "||false" in there...

? For non-x64 it is "||\n false"

derekbruening · 2017-03-02T15:44:13Z

core/arch/aarchxx/mangle.c

+                /* str r(...), [sp, #(i * 4)] */
+                PRE(ilist, instr, XINST_CREATE_store
+                    (dcontext, opnd_create_base_disp(DR_REG_XSP, DR_REG_NULL, 0,
+                                                     i * 4, OPSZ_PTR), arg));


That's weird. I thought I'd searched for all suspicious mentions of "4" and "8"...

derekbruening · 2017-03-02T15:47:01Z

core/arch/aarchxx/mangle.c

-    instr_t *mark = INSTR_CREATE_label(dcontext);
-    PRE(ilist, instr, mark);
+    uint num_regs = num_args < NUM_REGPARM ? num_args : NUM_REGPARM;
+    signed char regs[NUM_REGPARM];


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.

derekbruening · 2017-03-02T15:52:16Z

core/arch/aarchxx/mangle.c

-    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);
+    /* We expect every arg to be an integer or a full-size register. */


What about a memory location? The docs do not say it's disallowed, and as we have to copy the arg into a reg anyway, a user is likely to assume we support it. I was assuming we supported memory args (and it was just NYI, as the existing code shows). Certainly we do on x86.

Forgot to mention this in the review summary as this is a more major thing: current docs do not say memory args are disallowed. Is that what we want to do, update the docs to disallow them?

I don't think it would be hard to add memory operands, but then they should also be a test of memory operands, which would probably mean porting "cleancall", so probably that should be a separate commit.

I'm slightly unsure about which memory operands should be accepted:

constant offset from a register: Yes.
register offset from a register: A pain to implement but hard to justify excluding.
REL_ADDR_kind: Is this useful? (It would be AArch32-only.)
MEM_INSTR_kind: Does that make any sense here?
ABS_ADDR_kind: Does not exist for ARM/AArch64.

derekbruening · 2017-03-02T15:53:59Z

core/arch/aarchxx/mangle.c

-                    (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
+        ASSERT(opnd_is_immed_int((args[i])) ||


If a user is likely to break this, this should be a CLIENT_ASSERT, not an internal sanity check. However, see above: we should either support memory args (seems simple enough if they're < NUM_REGPARM), or we need to document that they are not supported.

So for this commit I should probably do a CLIENT_ASSERT that also accepts memory args, followed by an ASSERT_NOT_IMPLEMENTED that rejects the memory args.

derekbruening · 2017-03-02T16:20:36Z

core/arch/aarchxx/mangle.c

+        }
+    } while (i < num_regs);
+
+    /* From now on it is safe to use LR as a temporary. */


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.

derekbruening · 2017-03-02T16:26:50Z

core/arch/aarchxx/mangle.c

+        PRE(ilist, instr, XINST_CREATE_move(dcontext, opnd_create_reg(DR_REG_LR),
+                                            opnd_create_reg(regparms[i])));
+        do {
+            tmp = regs[i];


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.

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

derekbruening · 2017-03-02T16:27:16Z

core/arch/aarchxx/mangle.c

+        } while (tmp != first);
+    }
+
+    /* Set up stack arguments that are (non-zero) constants or SP. */


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

No guarantee of a free register earlier.

A comment should explain this: it was not obvious.

I've added a comment near the start of the function: /* The strategy here is to first set up ... (Yes, I do split infinitives!)

derekbruening · 2017-03-02T16:28:08Z

core/arch/aarchxx/mangle.c

    }
-    return 0;
+
+    return (uint)stack_inc;


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

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...

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.

derekbruening · 2017-03-02T16:28:54Z

core/arch/mangle_shared.c

+#elif defined(AARCHXX)
+        PRE(ilist, instr, INSTR_CREATE_add(dcontext, opnd_create_reg(REG_XSP),
+                                           opnd_create_reg(REG_XSP),
+                                           OPND_CREATE_INT32(stack_for_params)));


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

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

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

But XINST_CREATE_add() will generate an OP_lea on X86?

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.

The implementation should efficiently handle any (reasonable) number of arguments that are immediate integers or full-size registers. Test cases are added in cleancallparams.dll.c. Memory arguments are not yet implemented.

derekbruening

Missing FIXME/XXX i# comments but otherwise looks good

derekbruening · 2017-03-03T14:53:41Z

core/arch/aarchxx/mangle.c

@@ -596,20 +595,34 @@ insert_parameter_preparation(dcontext_t *dcontext, instrlist_t *ilist, instr_t *
    ptr_int_t stack_inc = 0;
    uint i, j;

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


Nice to have i# and XXX or FIXME

derekbruening · 2017-03-03T14:53:56Z

core/arch/aarchxx/mangle.c

+                       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]));


Best to have XXX or FIXME with i#

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

0efa381

The implementation should efficiently handle any (reasonable) number of arguments that are immediate integers or full-size registers. Test cases are added in cleancallparams.dll.c. Fixes #2210

egrimley requested a review from derekbruening March 2, 2017 11:44

derekbruening requested changes Mar 2, 2017

View reviewed changes

derekbruening approved these changes Mar 3, 2017

View reviewed changes

egrimley-arm added 4 commits March 3, 2017 15:13

Add FIXME.

9f4193f

Merge branch 'master' into i2210-aarchxx-clean-call

e160b1f

Merge branch 'master' into i2210-aarchxx-clean-call

4b4f331

Merge branch 'master' into i2210-aarchxx-clean-call

c99c52f

egrimley merged commit e06883c into master Mar 5, 2017

egrimley deleted the i2210-aarchxx-clean-call branch March 5, 2017 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

egrimley commented Mar 2, 2017

derekbruening left a comment

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

derekbruening Mar 2, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

egrimley Mar 3, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 3, 2017

egrimley Mar 3, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

egrimley Mar 3, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

egrimley Mar 2, 2017

derekbruening Mar 2, 2017

derekbruening left a comment

derekbruening Mar 3, 2017

derekbruening Mar 3, 2017

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

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

Conversation

egrimley commented Mar 2, 2017

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment