[5/7] arm: Move condition-failed codepath generation out of if()

Move the code to generate the "condition failed" instruction
codepath out of the if (singlestepping) {} else {}. This
will allow adding support for handling a new is_jmp type
which can't be neatly split into "singlestepping case"
versus "not singlestepping case".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 target/arm/translate.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.7.4

Hi Peter,

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> Move the code to generate the "condition failed" instruction

> codepath out of the if (singlestepping) {} else {}. This

> will allow adding support for handling a new is_jmp type

> which can't be neatly split into "singlestepping case"

> versus "not singlestepping case".


This makes also the code more readable :)

>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  target/arm/translate.c | 24 +++++++++++-------------

>  1 file changed, 11 insertions(+), 13 deletions(-)

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index a1a0e73..87fd702 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -11988,9 +11988,9 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)

>      /* At this stage dc->condjmp will only be set when the skipped

>         instruction was a conditional branch or trap, and the PC has

>         already been written.  */

> +    gen_set_condexec(dc);

>      if (unlikely(cs->singlestep_enabled || dc->ss_active)) {

>          /* Unconditional and "condition passed" instruction codepath. */

> -        gen_set_condexec(dc);

>          switch (dc->is_jmp) {

>          case DISAS_SWI:

>              gen_ss_advance(dc);

> @@ -12013,13 +12013,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)

>              /* FIXME: Single stepping a WFI insn will not halt the CPU. */

>              gen_singlestep_exception(dc);

>          }

> -        if (dc->condjmp) {

> -            /* "Condition failed" instruction codepath. */

> -            gen_set_label(dc->condlabel);

> -            gen_set_condexec(dc);

> -            gen_set_pc_im(dc, dc->pc);

> -            gen_singlestep_exception(dc);

> -        }

>      } else {

>          /* While branches must always occur at the end of an IT block,

>             there are a few other things that can cause us to terminate

> @@ -12029,7 +12022,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)

>              - Hardware watchpoints.

>             Hardware breakpoints have already been handled and skip this code.

>           */

> -        gen_set_condexec(dc);

>          switch(dc->is_jmp) {

>          case DISAS_NEXT:

>              gen_goto_tb(dc, 1, dc->pc);

> @@ -12069,11 +12061,17 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)

>              gen_exception(EXCP_SMC, syn_aa32_smc(), 3);

>              break;

>          }

> -        if (dc->condjmp) {

> -            gen_set_label(dc->condlabel);

> -            gen_set_condexec(dc);

> +    }

> +

> +    if (dc->condjmp) {

> +        /* "Condition failed" instruction codepath for the branch/trap insn */

> +        gen_set_label(dc->condlabel);

> +        gen_set_condexec(dc);

> +        if (unlikely(cs->singlestep_enabled || dc->ss_active)) {


I'm custom to think "let's change that and think how to protect the code 
to help the next one who will modify it" and wonder if it isn't safer to 
define:

const bool singlestepping;

singlestepping = cs->singlestep_enabled || dc->ss_active;

Then use:

if unlikely(singlestepping)

At line 11993 and here, what do you think?

Either way:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


> +            gen_set_pc_im(dc, dc->pc);

> +            gen_singlestep_exception(dc);

> +        } else {

>              gen_goto_tb(dc, 1, dc->pc);

> -            dc->condjmp = 0;

>          }

>      }

>

>

On 10 April 2017 at 14:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I'm custom to think "let's change that and think how to protect the code to

> help the next one who will modify it" and wonder if it isn't safer to

> define:

>

> const bool singlestepping;

>

> singlestepping = cs->singlestep_enabled || dc->ss_active;

>

> Then use:

>

> if unlikely(singlestepping)

>

> At line 11993 and here, what do you think?


I think we could do this with a function (that takes the DisasContext*);
then we can use it in the new gen_bx_excret_final_code() too.

I'll add an extra patch to the set in v2 that does that.

thanks
-- PMM