On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana....@googlemail.com> wrote:
> 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?
>>
Hi Kyrill,
Did you test --with-mode=thumb?
When using arm mode, I see regressions:
gcc.target/arm/neon-nested-apcs.c (test for excess errors)
gcc.target/arm/nested-apcs.c (test for excess errors)
Christophe
>> 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
