i#1551 Port DynamoRIO to ARM: split fcache_enter and fcache_return gencode

i#1551 Port DynamoRIO to ARM: split fcache_enter and fcache_return gencode
- rename core/arch/emit_utils.c to core/arch/emit_utils_shared.c
- add core/arch/{x86/arm}/emit_utils.c
- split fcache_enter and fcache_return gencode

R=bruening@google.com
BUG=1551

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

[zhaoqin, 2014-11-10 20:10:32]

could you please take a review :)
I first create it for self-review, and now should be ready for review.

[bruening, 2014-11-11 04:16:16]

I have mixed feelings I guess, as the original listings seemed kind of nice in
one place, and the original routines don't seem all that long.  But if we have
to add ARM-specific code it will get uglier if all in one place: but OTOH if
we're adding a bunch of ARM-specific code we should instead refactor into
separate routines in x86/ and arm/.  What is the plan for adding the ARM code? 
Is it just going to be big ifdefs with two parts inside each routine like the
save-gpr routine?  If so, why not move that routine into x86/ now?

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c
File core/arch/emit_utils.c (left):

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ol...
core/arch/emit_utils.c:3290: endif
I guess I liked having the full listing in one place

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c
File core/arch/emit_utils.c (right):

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3200: append_prepare_fcache_enter(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
It looks like this routine can't easily be re-used, esp with that name.  How
about a more descriptive name like append_move_dcontext_to_reg() or sthg.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3228: append_setup_target(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute, bool shared)
Name: what target?  append_setup_fcache_target()?  append_move_target_to_tls()?

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3354: append_restore_ext(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
Name doesn't seem clear to me

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3385: /* do nothing */
bug: q4-q7 are callee-saved so they need to be saved and restored.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3423: append_restore_gpr(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
Something like this seems like it could perhaps be used somewhere else and is
worth splitting out if so?

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3617: #endif /* X86_64 */
nit: The ifdef is right there, do we really need this?

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3854: /* NOTE: special handling the register for TLS */
?  I don't understand the comment.  Please tweak to make it clear whether it's
talking about what already exists, what doesn't exist, or what.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3903: /* do nothing on ARM */
bug: q4-q7 need to be saved

[zhaoqin, 2014-11-11 23:31:36]

it looks like the code we should move to x86/arm are the ext_reg and gpr
save/restore code.
The rest seems can be shared.
But I still like to split the code so it very clear to me each step of
enter/return the code cache.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c
File core/arch/emit_utils.c (left):

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ol...
core/arch/emit_utils.c:3290: endif
On 2014/11/11 04:16:16, bruening wrote:
> I guess I liked having the full listing in one place

add comment back.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c
File core/arch/emit_utils.c (right):

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3200: append_prepare_fcache_enter(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
On 2014/11/11 04:16:16, bruening wrote:
> It looks like this routine can't easily be re-used, esp with that name.  How
> about a more descriptive name like append_move_dcontext_to_reg() or sthg.

I am not sure about, it has a strong assumption that dcontext is in ARG1, which
might be only true for fcache_enter.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3228: append_setup_target(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute, bool shared)
On 2014/11/11 04:16:16, bruening wrote:
> Name: what target?  append_setup_fcache_target()? 
append_move_target_to_tls()?

use append_setup_fcache_target

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3354: append_restore_ext(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
On 2014/11/11 04:16:16, bruening wrote:
> Name doesn't seem clear to me

Done.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3385: /* do nothing */
On 2014/11/11 04:16:16, bruening wrote:
> bug: q4-q7 are callee-saved so they need to be saved and restored.

Done.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3423: append_restore_gpr(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
On 2014/11/11 04:16:16, bruening wrote:
> Something like this seems like it could perhaps be used somewhere else and is
> worth splitting out if so?

For this one, I guess I would move it to x86/ and have corresponding arm/ ones.
could merge with the ext_reg one, those are too arch specific.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3617: #endif /* X86_64 */
On 2014/11/11 04:16:16, bruening wrote:
> nit: The ifdef is right there, do we really need this?

Done.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3854: /* NOTE: special handling the register for TLS */
On 2014/11/11 04:16:16, bruening wrote:
> ?  I don't understand the comment.  Please tweak to make it clear whether it's
> talking about what already exists, what doesn't exist, or what.

Done.

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3903: /* do nothing on ARM */
On 2014/11/11 04:16:16, bruening wrote:
> bug: q4-q7 need to be saved

Done.

[bruening, 2014-11-11 23:38:39]

On Tue, Nov 11, 2014 at 6:31 PM, <zhaoqin@google.com> wrote:

> it looks like the code we should move to x86/arm are the ext_reg and gpr
> save/restore code.
> The rest seems can be shared.
> But I still like to split the code so it very clear to me each step of
> enter/return the code cache.


OK.

[bruening, 2014-11-12 05:45:55]

LGTM w/ comments

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c
File core/arch/emit_utils.c (right):

https://codereview.appspot.com/173880043/diff/90001/core/arch/emit_utils.c#ne...
core/arch/emit_utils.c:3200: append_prepare_fcache_enter(dcontext_t *dcontext,
instrlist_t *ilist, bool absolute)
On 2014/11/11 23:31:36, zhaoqin wrote:
> On 2014/11/11 04:16:16, bruening wrote:
> > It looks like this routine can't easily be re-used, esp with that name.  How
> > about a more descriptive name like append_move_dcontext_to_reg() or sthg.
> 
> I am not sure about, it has a strong assumption that dcontext is in ARG1,
which
> might be only true for fcache_enter.

Then I would say it should be part of the main fcache enter routine.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c
File core/arch/emit_utils.c (right):

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3386: /* s16–s31 (d8–d15, q4–q7) are caller save */
comment bug: callee-saved

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3608: endif
Wait, but this is now duplicated?  Hmm.  OK.

Also, shouldn't this comment make it clear which helper is responsible for which
sequence?

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3659: * the code is splitted into several help functions:
grammar: "split"

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3959: /* NOTE: we steal register (R10) for TLS access,
Shouldn't it be FIXME and not NOTE?

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:4010: /* s16–s31 (d8–d15, q4–q7) are caller save */
callee-saved

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:4218: if (absolute)
Ditto: can you label which sequence goes with which helper

[zhaoqin, 2014-11-13 22:20:49]

rename and move some code in emit_utils.c
should you take another look?

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c
File core/arch/emit_utils.c (right):

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3386: /* s16–s31 (d8–d15, q4–q7) are caller save */
On 2014/11/12 05:45:54, bruening wrote:
> comment bug: callee-saved

Done.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3608: endif
On 2014/11/12 05:45:54, bruening wrote:
> Wait, but this is now duplicated?  Hmm.  OK.
> 
> Also, shouldn't this comment make it clear which helper is responsible for
which
> sequence?

Done.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3659: * the code is splitted into several help functions:
On 2014/11/12 05:45:54, bruening wrote:
> grammar: "split"

Done.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:3959: /* NOTE: we steal register (R10) for TLS access,
On 2014/11/12 05:45:54, bruening wrote:
> Shouldn't it be FIXME and not NOTE?

Done.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:4010: /* s16–s31 (d8–d15, q4–q7) are caller save */
On 2014/11/12 05:45:55, bruening wrote:
> callee-saved

Done.

https://codereview.appspot.com/173880043/diff/130001/core/arch/emit_utils.c#n...
core/arch/emit_utils.c:4218: if (absolute)
On 2014/11/12 05:45:54, bruening wrote:
> Ditto: can you label which sequence goes with which helper

Done.

[bruening, 2014-11-13 22:43:27]

LGTM w/ suggestions

https://codereview.appspot.com/173880043/diff/210001/core/arch/arch.h
File core/arch/arch.h (right):

https://codereview.appspot.com/173880043/diff/210001/core/arch/arch.h#newcode...
core/arch/arch.h:1138: /* fcache_enter_common helper functions */
Are these the ones in arch-specific dirs?  Why not say that in the comment?

https://codereview.appspot.com/173880043/diff/210001/core/arch/arch.h#newcode...
core/arch/arch.h:1147: append_restore_ext_reg(dcontext_t *dcontext, instrlist_t
*ilist, bool absolute);
"ext_reg" still seems hard to understand (I have not seen that term used
anywhere else): how about "simd_regs"

https://codereview.appspot.com/173880043/diff/210001/core/arch/emit_utils_sha...
File core/arch/emit_utils_shared.c (right):

https://codereview.appspot.com/173880043/diff/210001/core/arch/emit_utils_sha...
core/arch/emit_utils_shared.c:39: /* FIXME i#1551: update code cache after
update on ARM because the hardware
Please add a separate line between the two comments

https://codereview.appspot.com/173880043/diff/210001/core/arch/emit_utils_sha...
core/arch/emit_utils_shared.c:3607: APP(ilist, SAVE_TO_DC(dcontext,
SCRATCH_REG0, LAST_EXIT_OFFSET));
Couldn't this just be put into the main routine?

[zhaoqin, 2014-11-13 23:33:45]

Committed patchset #12 (id:210001) manually as 2966 (presubmit successful).