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

[Kyrill Tkachov]

Hi Christophe,

On 12/05/16 20:57, Christophe Lyon wrote:
> On 12 May 2016 at 11:48, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
>> On Thu, May 5, 2016 at 12:50 PM, Kyrill Tkachov
>> <kyrylo.tkachov@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)

It's because I have a local patch in my binutils that makes gas warn on the
deprecated sequences that these two tests generate (they use the deprecated -mapcs option),
so these tests were already showing the (test for excess errors) FAIL for me,
so I they didn't appear in my tests diff for this patch. :(

I've reproduced the failure with a clean tree.
Where before we generated:
     ldm    sp, {fp, sp, pc}
now we generate:
     pop    {fp, sp, pc}

which are not equivalent (pop performs a write-back) and gas warns:
Warning: writeback of base register when in register list is UNPREDICTABLE

I'm testing a patch to fix this.
Sorry for the regression.
Kyrill

> Christophe
>
>>> 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.
>>
>> OK for affected branches and trunk  - thanks for fixing this and sorry
>> about the breakage.
>>
>> Ramana