i#1569 AArch64: Fix drx_buf on AArch64.

Commit log for first patchset:
---------------
i#1569 AArch64: Fix drx_buf on AArch64.

New macros in instr_create.h, make insert_parameter_preparation accept
32-bit arguments, and adapt drx_buf implementation and test program.
---------------

[zhaoqin, 2016-09-22 18:23:40]

bug on arg passed to reg_resize_to_opsz.

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_create.h
File core/arch/aarch64/instr_create.h (right):

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_creat...
core/arch/aarch64/instr_create.h:97: #define XINST_CREATE_store(dc, m, r)
INSTR_CREATE_str(dc, m, \
better to keep ()?

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_creat...
core/arch/aarch64/instr_create.h:297: instr_create_1dst_1src((dc), OP_strb, mem,
rt)
why change from Rt to rt?

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
suite/tests/client-interface/drx_buf-test.dll.c:325: scratch =
reg_resize_to_opsz(scratch, OPSZ_8);
why 8, will it works on ARM?

[Edmund.Grimley.Evans, 2016-09-22 18:42:31]

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_create.h
File core/arch/aarch64/instr_create.h (right):

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_creat...
core/arch/aarch64/instr_create.h:97: #define XINST_CREATE_store(dc, m, r)
INSTR_CREATE_str(dc, m, \
On 2016/09/22 18:23:40, zhaoqin wrote:
> better to keep ()?

I think it's not required.

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_creat...
core/arch/aarch64/instr_create.h:297: instr_create_1dst_1src((dc), OP_strb, mem,
rt)
On 2016/09/22 18:23:40, zhaoqin wrote:
> why change from Rt to rt?

Lower case is easier to type! This file generally needs tidying up...

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
suite/tests/client-interface/drx_buf-test.dll.c:325: scratch =
reg_resize_to_opsz(scratch, OPSZ_8);
On 2016/09/22 18:23:40, zhaoqin wrote:
> why 8, will it works on ARM?

Presumably I tested this and it did work, presumably because the ARM encoder
isn't too fussy. But you're right: it's wrong. I should probably correct it to
OPSZ_PTR.

[zhaoqin, 2016-09-22 18:52:03]

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_create.h
File core/arch/aarch64/instr_create.h (right):

https://codereview.appspot.com/310850043/diff/1/core/arch/aarch64/instr_creat...
core/arch/aarch64/instr_create.h:297: instr_create_1dst_1src((dc), OP_strb, mem,
rt)
On 2016/09/22 18:42:30, Edmund.Grimley.Evans wrote:
> On 2016/09/22 18:23:40, zhaoqin wrote:
> > why change from Rt to rt?
> 
> Lower case is easier to type! This file generally needs tidying up...

I do not have a very strong feeling about it.
I remember ARM manual used Rt, Rn, Rm.

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
suite/tests/client-interface/drx_buf-test.dll.c:325: scratch =
reg_resize_to_opsz(scratch, OPSZ_8);
On 2016/09/22 18:42:30, Edmund.Grimley.Evans wrote:
> On 2016/09/22 18:23:40, zhaoqin wrote:
> > why 8, will it works on ARM?
> 
> Presumably I tested this and it did work, presumably because the ARM encoder
> isn't too fussy. But you're right: it's wrong. I should probably correct it to
> OPSZ_PTR.

Hmm, I would expect an ASSERT based on the code at
https://github.com/DynamoRIO/dynamorio/blob/bed62b27ffeaf22851e3fce1c20535627...

Also, could you please add comment about why using OPSZ_PTR (due to ARM/AArch64
encoding rule?) since the test here would like to test OPSZ_4.

[Edmund.Grimley.Evans, 2016-09-22 19:02:04]

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/1/suite/tests/client-interface/...
suite/tests/client-interface/drx_buf-test.dll.c:325: scratch =
reg_resize_to_opsz(scratch, OPSZ_8);
On 2016/09/22 18:52:03, zhaoqin wrote:
> Also, could you please add comment about why using OPSZ_PTR (due to
ARM/AArch64
> encoding rule?) since the test here would like to test OPSZ_4.

I see. What I should probably do is resize the reg to OPSZ_PTR for calling
instrlist_insert_mov_immed_ptrsz, then resize it to OPSZ_4 for calling
drx_buf_insert_buf_store. I'll test this on all architectures and send an
updated patch.

[Edmund.Grimley.Evans, 2016-09-23 10:33:35]

Commit log for latest patchset:
---------------
i#1569 AArch64: Fix drx_buf on AArch64.

New macros in instr_create.h, make insert_parameter_preparation accept
32-bit arguments, and adapt drx_buf implementation and test program.

Review-URL: https://codereview.appspot.com/310850043
---------------

[zhaoqin, 2016-09-26 19:37:34]

need some clarification.

https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c
File ext/drx/drx_buf.c (right):

https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c#newcode549
ext/drx/drx_buf.c:549: instrlist_insert_mov_immed_ptrsz(drcontext, 0x48474645,
0x48474645?
Shouldn't this be opnd_get_immed_int(opnd) or something?

https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
suite/tests/client-interface/drx_buf-test.dll.c:318: (scratch, OPSZ_PTR)),
Why do we move the scratch computation around instead of just replacing OPSZ_8
with OPSZ_PTR

[Edmund.Grimley.Evans, 2016-09-26 20:38:08]

https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c
File ext/drx/drx_buf.c (right):

https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c#newcode549
ext/drx/drx_buf.c:549: instrlist_insert_mov_immed_ptrsz(drcontext, 0x48474645,
On 2016/09/26 19:37:34, zhaoqin wrote:
> 0x48474645?
> Shouldn't this be opnd_get_immed_int(opnd) or something?

Yes, it looks like it. What a weird thing for me to have written...

https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
File suite/tests/client-interface/drx_buf-test.dll.c (right):

https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
suite/tests/client-interface/drx_buf-test.dll.c:318: (scratch, OPSZ_PTR)),
On 2016/09/26 19:37:34, zhaoqin wrote:
> Why do we move the scratch computation around instead of just replacing OPSZ_8
> with OPSZ_PTR

I think "scratch" should be OPSZ_4 for the drx_buf_insert_buf_store just below,
but it has to be OPSZ_PTR for the argument to instrlist_insert_mov_immed_ptrsz.

[Edmund.Grimley.Evans, 2016-09-27 08:09:38]

Commit log for latest patchset:
---------------
i#1569 AArch64: Fix drx_buf on AArch64.

New macros in instr_create.h, make insert_parameter_preparation accept
32-bit arguments, and adapt drx_buf implementation and test program.

Review-URL: https://codereview.appspot.com/310850043
---------------

[zhaoqin, 2016-09-30 15:56:08]

On 2016/09/26 20:38:08, Edmund.Grimley.Evans wrote:
> https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c
> File ext/drx/drx_buf.c (right):
> 
>
https://codereview.appspot.com/310850043/diff/20001/ext/drx/drx_buf.c#newcode549
> ext/drx/drx_buf.c:549: instrlist_insert_mov_immed_ptrsz(drcontext, 0x48474645,
> On 2016/09/26 19:37:34, zhaoqin wrote:
> > 0x48474645?
> > Shouldn't this be opnd_get_immed_int(opnd) or something?
> 
> Yes, it looks like it. What a weird thing for me to have written...
> 
>
https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
> File suite/tests/client-interface/drx_buf-test.dll.c (right):
> 
>
https://codereview.appspot.com/310850043/diff/20001/suite/tests/client-interf...
> suite/tests/client-interface/drx_buf-test.dll.c:318: (scratch, OPSZ_PTR)),
> On 2016/09/26 19:37:34, zhaoqin wrote:
> > Why do we move the scratch computation around instead of just replacing
OPSZ_8
> > with OPSZ_PTR
> 
> I think "scratch" should be OPSZ_4 for the drx_buf_insert_buf_store just
below,
> but it has to be OPSZ_PTR for the argument to
instrlist_insert_mov_immed_ptrsz.

I see, that make sense, also a lot of scratch usage above this line may need it
to be OPSZ_4.
Some comment about why it has to be OPSZ_PTR here might be helpful.

[zhaoqin, 2016-09-30 15:56:15]

LGTM

[Edmund.Grimley.Evans, 2016-10-03 08:50:20]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/de022285fbf16f6c6bbd3b4aac49c44...

Final commit log: 
---------------
i#1569 AArch64: Fix drx_buf on AArch64.

New macros in instr_create.h, make insert_parameter_preparation accept
32-bit arguments, and adapt drx_buf implementation and test program.

Review-URL: https://codereview.appspot.com/310850043
---------------