i#1551 Port DynamoRIO to ARM: update instrument.c to build on ARM

i#1551 Port DynamoRIO to ARM: update instrument.c to build on ARM
- add TLS_SLOT_R* and TLS_REG_R*
- add INSTR_CREATE_load and INSTR_CREATE_store
- replace DR_REG_STACK_PTR with DR_REG_XSP for cross-platform stack pointer
- add dr_save_arith_flags_to_reg and dr_restore_arith_flags_from_reg
- update instrument.c to build on ARM

R=bruening@google.com
BUG=1551

Committed: https://code.google.com/p/dynamorio/source/detail?r=2923

[bruening, 2014-10-28 19:25:24]

"instruemnt.c"

[zhaoqin, 2014-10-28 19:56:43]

On 2014/10/28 19:25:24, bruening wrote:
> "instruemnt.c"

updated

[bruening, 2014-10-29 14:38:03]

On 2014/10/28 19:56:43, zhaoqin wrote:
> On 2014/10/28 19:25:24, bruening wrote:
> > "instruemnt.c"
> 
> updated

It's also in the title

[zhaoqin, 2014-10-29 14:56:33]

On 2014/10/29 14:38:03, bruening wrote:
> On 2014/10/28 19:56:43, zhaoqin wrote:
> > On 2014/10/28 19:25:24, bruening wrote:
> > > "instruemnt.c"
> > 
> > updated
> 
> It's also in the title

updated

[bruening, 2014-10-29 16:35:25]

A few issues, might be worth another look

https://codereview.appspot.com/166810043/diff/80001/api/docs/release.dox
File api/docs/release.dox (right):

https://codereview.appspot.com/166810043/diff/80001/api/docs/release.dox#newc...
api/docs/release.dox:165: - Added INSTR_CREATE_load()
Might be nicer to group all the INSTR_CREATE together

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_create.h
File core/arch/arm/instr_create.h (left):

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:51: opnd_create_rel_addr(addr, size)
What happened to this?  PC-relative refs exist on ARM, so it seems like this
should stay how it was.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_create.h
File core/arch/arm/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:53: /* FIXME i#1551: provide documentation for
INSTR_CREATE_* macros */
Are you delaying adding the docs b/c you're assuming some script is going to
auto-fill them in later?

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:63: /* FIXME i#1551: provide cross-platform
INSTR_CREATE_* macros */
What does this comment mean?  Aren't these add and sub cross-platform macros? 
Do you mean, "provide _more_"?

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:103: #define INSTR_CREATE_store(dc, mem, reg) 
INSTR_CREATE_ldr(dc, mem, reg)
bug: ldr???

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_create.h
File core/arch/x86/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_creat...
core/arch/x86/instr_create.h:3699: 
Please add a comment separator labeling these as the cross-platform macros.

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_creat...
core/arch/x86/instr_create.h:3699: 
It makes more sense to me to put these at the top of the file

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:4894: REG_NULL, 0, 0,
Looks like both changes should be reverted?

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5063: DR_REG_R6, DR_REG_R5, DR_REG_R4, DR_REG_R3,
DR_REG_R2, DR_REG_R1
Why not r7?

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5378: "only xax should be used for save arith flags in
X86");
The docs don't say this!  They should.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5406: "only xax should be used for save arith flags in
X86");
The docs need to explain this

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5574: ASSERT_NOT_IMPLEMENTED(false);
For the future: we may want to split these out into arch/{x86,arm}/ files?

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h
File core/lib/instrument.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4653: */
These docs should be updated to be cross-platform

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4666: */
These docs should be updated to be cross-platform

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4687: dr_save_arith_flags_to_xax(void *drcontext,
instrlist_t *ilist, instr_t *where);
So you made a whole new routine with near-identical docs, because this has the
name "xax" in it?   Hmm.  Note that I kept the prefix EFLAGS_ for the ARM
constants to avoid too much churn.  I guess you have to take in a register, so
it's not just the name (if it were just the name I'd say have a #define or
sthg).

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4723: * most likely from dr_save_status_to_reg().
bug

[zhaoqin, 2014-10-29 21:59:10]

PTAL

https://codereview.appspot.com/166810043/diff/80001/api/docs/release.dox
File api/docs/release.dox (right):

https://codereview.appspot.com/166810043/diff/80001/api/docs/release.dox#newc...
api/docs/release.dox:165: - Added INSTR_CREATE_load()
On 2014/10/29 16:35:23, bruening wrote:
> Might be nicer to group all the INSTR_CREATE together

Done.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_create.h
File core/arch/arm/instr_create.h (left):

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:51: opnd_create_rel_addr(addr, size)
On 2014/10/29 16:35:23, bruening wrote:
> What happened to this?  PC-relative refs exist on ARM, so it seems like this
> should stay how it was.

opnd_create_rel_addr is X64 only.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_create.h
File core/arch/arm/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:53: /* FIXME i#1551: provide documentation for
INSTR_CREATE_* macros */
On 2014/10/29 16:35:23, bruening wrote:
> Are you delaying adding the docs b/c you're assuming some script is going to
> auto-fill them in later?

We probably will have a doxygen group documentation, so do not both to add it
now.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:63: /* FIXME i#1551: provide cross-platform
INSTR_CREATE_* macros */
On 2014/10/29 16:35:23, bruening wrote:
> What does this comment mean?  Aren't these add and sub cross-platform macros? 
> Do you mean, "provide _more_"?

These are not general enough, for ARM we cannot use it for
add r1, r2, r3, so not sure if they should be listed cross-platform.

also the eflags result would be different.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:103: #define INSTR_CREATE_store(dc, mem, reg) 
INSTR_CREATE_ldr(dc, mem, reg)
On 2014/10/29 16:35:23, bruening wrote:
> bug: ldr???

Done.

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_create.h
File core/arch/x86/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_creat...
core/arch/x86/instr_create.h:3699: 
On 2014/10/29 16:35:23, bruening wrote:
> It makes more sense to me to put these at the top of the file

Done.

https://codereview.appspot.com/166810043/diff/80001/core/arch/x86/instr_creat...
core/arch/x86/instr_create.h:3699: 
On 2014/10/29 16:35:23, bruening wrote:
> Please add a comment separator labeling these as the cross-platform macros.

Done.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:4894: REG_NULL, 0, 0,
On 2014/10/29 16:35:24, bruening wrote:
> Looks like both changes should be reverted?

Done.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5063: DR_REG_R6, DR_REG_R5, DR_REG_R4, DR_REG_R3,
DR_REG_R2, DR_REG_R1
On 2014/10/29 16:35:24, bruening wrote:
> Why not r7?

To mirror the number of REGs in x86 32-bit.
Otherwise, has to change many other places like NUM_SPILL_SLOTS,

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5378: "only xax should be used for save arith flags in
X86");
On 2014/10/29 16:35:24, bruening wrote:
> The docs don't say this!  They should.

Done.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5406: "only xax should be used for save arith flags in
X86");
On 2014/10/29 16:35:24, bruening wrote:
> The docs need to explain this

Done.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.c#new...
core/lib/instrument.c:5574: ASSERT_NOT_IMPLEMENTED(false);
On 2014/10/29 16:35:24, bruening wrote:
> For the future: we may want to split these out into arch/{x86,arm}/ files?

Yes, it is very x86 specific. add comment.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h
File core/lib/instrument.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4653: */
On 2014/10/29 16:35:24, bruening wrote:
> These docs should be updated to be cross-platform

this is deprecated routine, so I would prefer leave it there, fewer people would
use it.
even update, I want to add it as x86 only.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4666: */
On 2014/10/29 16:35:24, bruening wrote:
> These docs should be updated to be cross-platform

ditto

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4687: dr_save_arith_flags_to_xax(void *drcontext,
instrlist_t *ilist, instr_t *where);
On 2014/10/29 16:35:24, bruening wrote:
> So you made a whole new routine with near-identical docs, because this has the
> name "xax" in it?   Hmm.  Note that I kept the prefix EFLAGS_ for the ARM
> constants to avoid too much churn.  I guess you have to take in a register, so
> it's not just the name (if it were just the name I'd say have a #define or
> sthg).

right, the reg makes me to add new routine instead.

https://codereview.appspot.com/166810043/diff/80001/core/lib/instrument.h#new...
core/lib/instrument.h:4723: * most likely from dr_save_status_to_reg().
On 2014/10/29 16:35:24, bruening wrote:
> bug

Done.

[bruening, 2014-10-30 02:21:40]

LGTM w/ comments.  Also, please update the commit message w/ the updated names.

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_create.h
File core/arch/arm/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/80001/core/arch/arm/instr_creat...
core/arch/arm/instr_create.h:63: /* FIXME i#1551: provide cross-platform
INSTR_CREATE_* macros */
On 2014/10/29 21:59:09, zhaoqin wrote:
> On 2014/10/29 16:35:23, bruening wrote:
> > What does this comment mean?  Aren't these add and sub cross-platform
macros? 
> > Do you mean, "provide _more_"?
> 
> These are not general enough, for ARM we cannot use it for
> add r1, r2, r3, so not sure if they should be listed cross-platform.
> 
> also the eflags result would be different.
> 
> 
> 

Please put that in the comment.

https://codereview.appspot.com/166810043/diff/100001/api/docs/release.dox
File api/docs/release.dox (right):

https://codereview.appspot.com/166810043/diff/100001/api/docs/release.dox#new...
api/docs/release.dox:167: - Added dr_restore_status_flags_from_reg().
Didn't you change these names?

https://codereview.appspot.com/166810043/diff/100001/core/arch/arm/instr_crea...
File core/arch/arm/instr_create.h (right):

https://codereview.appspot.com/166810043/diff/100001/core/arch/arm/instr_crea...
core/arch/arm/instr_create.h:96: 
Shouldn't you end the group?

It's worth looking at the generate docs as well to make sure everything is
kosher.

https://codereview.appspot.com/166810043/diff/100001/core/arch/opnd.h
File core/arch/opnd.h (right):

https://codereview.appspot.com/166810043/diff/100001/core/arch/opnd.h#newcode...
core/arch/opnd.h:1126: * \note For 64-bit X86 or 32-bit ARM DR builds only.
Why "32-bit"?  It should be all ARM.

https://codereview.appspot.com/166810043/diff/100001/core/lib/instrument.c
File core/lib/instrument.c (right):

https://codereview.appspot.com/166810043/diff/100001/core/lib/instrument.c#ne...
core/lib/instrument.c:5570: * Alsoe, we may want to split these out into
arch/{x86,arm}/ files
spelling

https://codereview.appspot.com/166810043/diff/100001/core/lib/instrument.h
File core/lib/instrument.h (right):

https://codereview.appspot.com/166810043/diff/100001/core/lib/instrument.h#ne...
core/lib/instrument.h:4708: * the arithmetic flags (6 arithmetic flags on X86 or
APSR on ARM) into \p reg.
"the the"

[zhaoqin, 2014-10-30 17:47:53]

Committed patchset #7 (id:120001) manually as 2923 (presubmit successful).