On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov
<kyrylo.tkac...@foss.arm.com> 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.tkac...@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.tkac...@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.
OK for affected branches and trunk - thanks for fixing this and sorry
about the breakage.
Ramana
