i#1569 AArch64: Create clean_call_opt_shared.c and refactor.

Commit log for first patchset:
---------------
i#1569 AArch64: Create clean_call_opt_shared.c and refactor.

About half of x86/clean_call_opt.c is moved to clean_call_opt_shared.c
with minor changes. Prototypes for a new internal API are in
clean_call_opt.h and {aarch64,arm}/clean_call_opt.c are rewritten
accordingly with ASSERT_NOT_IMPLEMENTED in function bodies.

This is based on work done by Kevin Zhou in July.
---------------

[Edmund.Grimley.Evans, 2016-12-05 09:51:33]

Ping.

[bruening, 2016-12-05 18:15:47]

https://codereview.appspot.com/311330043/diff/1/core/arch/arm/clean_call_opt.c
File core/arch/arm/clean_call_opt.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/arm/clean_call_opt....
core/arch/arm/clean_call_opt.c:43: ASSERT_NOT_IMPLEMENTED(false); /* FIXME
i#1551: NYI on ARM */
i#1551 is closed.  Best to file a separate issue for clean call work on ARM.  We
should have done so when we closed it for each outstanding piece now that
intense active ARM porting is done.  It is easier to track work with separate
issues once the code is declared release-ready and different (third-party or
otherwise) contributors may work on different pieces over longer time periods. 
We filed a few already but not all.  Xref the final comment in i#1551.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h
File core/arch/clean_call_opt.h (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:33: /* file "clean_call_opt.h" - arch-specific clean
call optimisation */
For us Americans we would prefer "optimization" :)

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:38: #define INIT_HTABLE_SIZE_CALLEE 6 /* should
remain small */
Why is this in the header?  Isn't it only needed in the shared .c file?

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:41: #define MAX_NUM_FUNC_INSTRS 4096
This appears unused: just remove it

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:42: /* the max number of instructions the callee can
have for inline. */
May as well fix the style violation here if you're changing the blame history
anyway.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:43: #define MAX_NUM_INLINE_INSTRS 20
Why is this in the header when it's only used in one place?

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:46: callee_info_reserve_slot(callee_info_t *ci,
slot_kind_t kind, reg_id_t value);
Please separate these (banner comment, perhaps) into those provided by _shared.c
and those expected to be implemented in the arch-specific file.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
File core/arch/clean_call_opt_shared.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:299: return NULL;
Can this be split into the arch-specific files?

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:299: return NULL;
This needs a FIXME comment: this must be temporary.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:538: opt_inline &=
check_callee_ilist_inline(dcontext, ci);
bug: bitwise and is not safe in C where the "bool" return value need not be 0 or
1

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:792: # ifdef  X86
double space

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:792: # ifdef  X86
Can this be moved to arch-specific code -- or at least a XXX comment about doing
so in the future

https://codereview.appspot.com/311330043/diff/1/core/arch/x86/clean_call_opt.c
File core/arch/x86/clean_call_opt.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/x86/clean_call_opt....
core/arch/x86/clean_call_opt.c:516: if (instr != NULL) opt_inline = false;
style violation

[bruening, 2016-12-05 18:27:02]

Meant to give a top-level comment that overall it looks fairly good: just some
minor issues and what looks like a bug

[bruening, 2016-12-06 05:40:15]

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h
File core/arch/clean_call_opt.h (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:33: /* file "clean_call_opt.h" - arch-specific clean
call optimisation */
On 2016/12/05 18:15:47, bruening wrote:
> For us Americans we would prefer "optimization" :)

(to clarify: not really asking for or expecting any change here)

[Edmund.Grimley.Evans, 2016-12-06 11:44:41]

https://codereview.appspot.com/311330043/diff/1/core/arch/arm/clean_call_opt.c
File core/arch/arm/clean_call_opt.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/arm/clean_call_opt....
core/arch/arm/clean_call_opt.c:43: ASSERT_NOT_IMPLEMENTED(false); /* FIXME
i#1551: NYI on ARM */
On 2016/12/05 18:15:47, bruening wrote:
> i#1551 is closed.  Best to file a separate issue for clean call work on ARM.

I've created i#2094 and changed the numbers in this file.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h
File core/arch/clean_call_opt.h (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:38: #define INIT_HTABLE_SIZE_CALLEE 6 /* should
remain small */
On 2016/12/05 18:15:47, bruening wrote:
> Why is this in the header?  Isn't it only needed in the shared .c file?

I can't think why I moved it. I'll put it back...

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:41: #define MAX_NUM_FUNC_INSTRS 4096
On 2016/12/05 18:15:47, bruening wrote:
> This appears unused: just remove it

Done.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:42: /* the max number of instructions the callee can
have for inline. */
On 2016/12/05 18:15:47, bruening wrote:
> May as well fix the style violation here if you're changing the blame history
> anyway.

I'll treat this as an honorary sentence!

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:43: #define MAX_NUM_INLINE_INSTRS 20
On 2016/12/05 18:15:47, bruening wrote:
> Why is this in the header when it's only used in one place?

I've moved it back.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt.h#ne...
core/arch/clean_call_opt.h:46: callee_info_reserve_slot(callee_info_t *ci,
slot_kind_t kind, reg_id_t value);
On 2016/12/05 18:15:47, bruening wrote:
> Please separate these (banner comment, perhaps) into those provided by
_shared.c
> and those expected to be implemented in the arch-specific file.

Banners inserted.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
File core/arch/clean_call_opt_shared.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:299: return NULL;
On 2016/12/05 18:15:47, bruening wrote:
> Can this be split into the arch-specific files?

I've moved it into check_callee_instr_level2.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:299: return NULL;
On 2016/12/05 18:15:47, bruening wrote:
> This needs a FIXME comment: this must be temporary.

Done.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:538: opt_inline &=
check_callee_ilist_inline(dcontext, ci);
On 2016/12/05 18:15:47, bruening wrote:
> bug: bitwise and is not safe in C where the "bool" return value need not be 0
or
> 1

Fixed.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:792: # ifdef  X86
On 2016/12/05 18:15:47, bruening wrote:
> Can this be moved to arch-specific code -- or at least a XXX comment about
doing
> so in the future

Added comment.

https://codereview.appspot.com/311330043/diff/1/core/arch/clean_call_opt_shar...
core/arch/clean_call_opt_shared.c:792: # ifdef  X86
On 2016/12/05 18:15:47, bruening wrote:
> double space

Fixed.

https://codereview.appspot.com/311330043/diff/1/core/arch/x86/clean_call_opt.c
File core/arch/x86/clean_call_opt.c (right):

https://codereview.appspot.com/311330043/diff/1/core/arch/x86/clean_call_opt....
core/arch/x86/clean_call_opt.c:516: if (instr != NULL) opt_inline = false;
On 2016/12/05 18:15:47, bruening wrote:
> style violation

Fixed.

[Edmund.Grimley.Evans, 2016-12-06 11:46:34]

Commit log for latest patchset:
---------------
i#1569 AArch64: Create clean_call_opt_shared.c and refactor.

About half of x86/clean_call_opt.c is moved to clean_call_opt_shared.c
with minor changes. Prototypes for a new internal API are in
clean_call_opt.h and {aarch64,arm}/clean_call_opt.c are rewritten
accordingly with ASSERT_NOT_IMPLEMENTED in function bodies.

This is based on work done by Kevin Zhou in July.

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

[bruening, 2016-12-06 16:26:20]

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
File core/arch/clean_call_opt_shared.c (right):

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
core/arch/clean_call_opt_shared.c:478: opt_inline &=
check_callee_ilist_inline(dcontext, ci) ? 1 : 0;
This is still fragile.  Imagine an earlier line that says "opt_inline =
helper_func()" which for some reason returns 2.  Now we do bitwise & with 1.

Better, safer, and clearer to use logical && for booleans and leave bitwise
operators for actual bit operations.

[Edmund.Grimley.Evans, 2016-12-06 17:27:39]

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
File core/arch/clean_call_opt_shared.c (right):

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
core/arch/clean_call_opt_shared.c:478: opt_inline &=
check_callee_ilist_inline(dcontext, ci) ? 1 : 0;
On 2016/12/06 16:26:20, bruening wrote:
> This is still fragile.  Imagine an earlier line that says "opt_inline =
> helper_func()" which for some reason returns 2.  Now we do bitwise & with 1.
> 
> Better, safer, and clearer to use logical && for booleans and leave bitwise
> operators for actual bit operations.

Sorry. For some reason I was assuming that opt_inline was an int. Presumably
this should be either

    if (!check_...)
        opt_inline = false;

or

    opt_inline = opt_inline && check_...(...);

which are different if the function has side effects...

[bruening, 2016-12-06 17:41:26]

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
File core/arch/clean_call_opt_shared.c (right):

https://codereview.appspot.com/311330043/diff/20001/core/arch/clean_call_opt_...
core/arch/clean_call_opt_shared.c:478: opt_inline &=
check_callee_ilist_inline(dcontext, ci) ? 1 : 0;
On 2016/12/06 17:27:39, Edmund.Grimley.Evans wrote:
> On 2016/12/06 16:26:20, bruening wrote:
> > This is still fragile.  Imagine an earlier line that says "opt_inline =
> > helper_func()" which for some reason returns 2.  Now we do bitwise & with 1.
> > 
> > Better, safer, and clearer to use logical && for booleans and leave bitwise
> > operators for actual bit operations.
> 
> Sorry. For some reason I was assuming that opt_inline was an int. Presumably
> this should be either
> 
>     if (!check_...)
>         opt_inline = false;
> 
> or
> 
>     opt_inline = opt_inline && check_...(...);
> 
> which are different if the function has side effects...

Or:

    opt_inline = check_...(...) && opt_inline;

[Edmund.Grimley.Evans, 2016-12-07 15:57:46]

Commit log for latest patchset:
---------------
i#1569 AArch64: Create clean_call_opt_shared.c and refactor.

About half of x86/clean_call_opt.c is moved to clean_call_opt_shared.c
with minor changes. Prototypes for a new internal API are in
clean_call_opt.h and {aarch64,arm}/clean_call_opt.c are rewritten
accordingly with ASSERT_NOT_IMPLEMENTED in function bodies.

This is based on work done by Kevin Zhou in July.

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

[bruening, 2016-12-07 16:54:48]

LGTM

[Edmund.Grimley.Evans, 2016-12-08 10:29:21]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/7ff49b0978805ef144ddaa0026cc596...

Final commit log: 
---------------
i#1569 AArch64: Create clean_call_opt_shared.c and refactor.

About half of x86/clean_call_opt.c is moved to clean_call_opt_shared.c
with minor changes. Prototypes for a new internal API are in
clean_call_opt.h and {aarch64,arm}/clean_call_opt.c are rewritten
accordingly with ASSERT_NOT_IMPLEMENTED in function bodies.

This is based on work done by Kevin Zhou in July.

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