[PATCH][ARM] PR target/70830: Avoid POP-{reglist}^ when returning from interrupt handlers

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00395.html

Thanks,
Kyrill

On 05/05/16 12:50, Kyrill Tkachov wrote:
> Hi all,
>
> In this PR we deal with some fallout from the conversion to unified assembly.
> We now end up emitting instructions like:
>   pop {r0,r1,r2,r3,pc}^
> which is not legal. We have to use an LDM form.
>
> There are bugs in two arm.c functions: output_return_instruction and arm_output_multireg_pop.
>
> In output_return_instruction the buggy hunk from the conversion was:
>           else
> -           if (TARGET_UNIFIED_ASM)
>               sprintf (instr, "pop%s\t{", conditional);
> -           else
> -             sprintf (instr, "ldm%sfd\t%%|sp!, {", conditional);
>
> The code was already very obscurely structured and arguably the bug was latent.
> It emitted POP only when TARGET_UNIFIED_ASM was on, and since TARGET_UNIFIED_ASM was on
> only for Thumb, we never went down this path interrupt handling code, since the interrupt
> attribute is only available for ARM code. After the removal of TARGET_UNIFIED_ASM we ended up
> using POP unconditionally. So this patch adds a check for IS_INTERRUPT and outputs the
> appropriate LDM form.
>
> In arm_output_multireg_pop the buggy hunk was:
> -  if ((regno_base == SP_REGNUM) && TARGET_THUMB)
> +  if ((regno_base == SP_REGNUM) && update)
>      {
> -      /* Output pop (not stmfd) because it has a shorter encoding.  */
> -      gcc_assert (update);
>        sprintf (pattern, "pop%s\t{", conditional);
>      }
>
> Again, the POP was guarded on TARGET_THUMB and so would never be taken on interrupt handling
> routines. This patch guards that with the appropriate check on interrupt return.
>
> Also, there are a couple of bugs in the 'else' branch of that 'if':
> * The "ldmfd%s" was output without a '\t' at the end which meant that the base register
> name would be concatenated with the 'ldmfd', creating invalid assembly.
>
> * The logic:
>
>       if (regno_base == SP_REGNUM)
>       /* update is never true here, hence there is no need to handle
>          pop here.  */
>     sprintf (pattern, "ldmfd%s", conditional);
>
>       if (update)
>     sprintf (pattern, "ldmia%s\t", conditional);
>       else
>     sprintf (pattern, "ldm%s\t", conditional);
>
> Meant that for "regno == SP_REGNUM && !update" we'd end up printing "ldmfd%sldm%s\t"
> to pattern. I didn't manage to reproduce that condition though, so maybe it can't ever occur.
> This patch fixes both these issues nevertheless.
>
> I've added the testcase from the PR to catch the fix in output_return_instruction.
> The testcase doesn't catch the bugs in arm_output_multireg_pop, but the existing tests
> gcc.target/arm/interrupt-1.c and gcc.target/arm/interrupt-2.c would have caught them
> if only they were assemble tests rather than just compile. So this patch makes them
> assembly tests (and reverts the scan-assembler checks for the correct LDM pattern).
>
> Bootstrapped and tested on arm-none-linux-gnueabihf.
> Ok for trunk and GCC 6?
>
> Thanks,
> Kyrill
>
> 2016-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/70830
>     * config/arm/arm.c (arm_output_multireg_pop): Avoid POP instruction
>     when popping the PC and within an interrupt handler routine.
>     Add missing tab to output of "ldmfd".
>     (output_return_instruction): Output LDMFD with SP update rather
>     than POP when returning from interrupt handler.
>
> 2016-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/70830
>     * gcc.target/arm/interrupt-1.c: Change dg-compile to dg-assemble.
>     Add -save-temps to dg-options.
>     Scan for ldmfd rather than pop instruction.
>     * gcc.target/arm/interrupt-2.c: Likewise.
>     * gcc.target/arm/pr70830.c: New test.