| Message ID | 572B337E.3040201@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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? > > 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
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) 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
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
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 5136bc0171c8cb1f670096cae93037cf4611c4c5..2f1de2bcb7d69889eb080e338f8e939cc038b63b 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -17582,6 +17582,7 @@ arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse, int num_saves = XVECLEN (operands[0], 0); unsigned int regno; unsigned int regno_base = REGNO (operands[1]); + bool interrupt_p = IS_INTERRUPT (arm_current_func_type ()); offset = 0; offset += update ? 1 : 0; @@ -17599,7 +17600,8 @@ arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse, } conditional = reverse ? "%?%D0" : "%?%d0"; - if ((regno_base == SP_REGNUM) && update) + /* Can't use POP if returning from an interrupt. */ + if ((regno_base == SP_REGNUM) && !(interrupt_p && return_pc)) { sprintf (pattern, "pop%s\t{", conditional); } @@ -17608,11 +17610,8 @@ arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse, /* Output ldmfd when the base register is SP, otherwise output ldmia. It's just a convention, their semantics are identical. */ 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, "ldmfd%s\t", conditional); + else if (update) sprintf (pattern, "ldmia%s\t", conditional); else sprintf (pattern, "ldm%s\t", conditional); @@ -17638,7 +17637,7 @@ arm_output_multireg_pop (rtx *operands, bool return_pc, rtx cond, bool reverse, strcat (pattern, "}"); - if (IS_INTERRUPT (arm_current_func_type ()) && return_pc) + if (interrupt_p && return_pc) strcat (pattern, "^"); output_asm_insn (pattern, &cond); @@ -19449,8 +19448,12 @@ output_return_instruction (rtx operand, bool really_return, bool reverse, sprintf (instr, "ldmfd%s\t%%|sp, {", conditional); } } + /* For interrupt returns we have to use an LDM rather than + a POP so that we can use the exception return variant. */ + else if (IS_INTERRUPT (func_type)) + sprintf (instr, "ldmfd%s\t%%|sp!, {", conditional); else - sprintf (instr, "pop%s\t{", conditional); + sprintf (instr, "pop%s\t{", conditional); p = instr + strlen (instr); diff --git a/gcc/testsuite/gcc.target/arm/interrupt-1.c b/gcc/testsuite/gcc.target/arm/interrupt-1.c index debbaf78cc84d17e3c43fbd9185f7d6b3fe5d88f..fe94877cead7501dd73f2eba92feff395de9df2f 100644 --- a/gcc/testsuite/gcc.target/arm/interrupt-1.c +++ b/gcc/testsuite/gcc.target/arm/interrupt-1.c @@ -1,8 +1,8 @@ /* Verify that prologue and epilogue are correct for functions with __attribute__ ((interrupt)). */ -/* { dg-do compile } */ +/* { dg-do assemble } */ /* { dg-require-effective-target arm_nothumb } */ -/* { dg-options "-O0 -marm" } */ +/* { dg-options "-O0 -marm -save-temps" } */ /* This test is not valid when -mthumb. */ extern void bar (int); @@ -14,4 +14,4 @@ void foo () } /* { dg-final { scan-assembler "push\t{r0, r1, r2, r3, r4, fp, ip, lr}" } } */ -/* { dg-final { scan-assembler "pop\t{r0, r1, r2, r3, r4, fp, ip, pc}\\^" } } */ +/* { dg-final { scan-assembler "ldmfd\tsp!, {r0, r1, r2, r3, r4, fp, ip, pc}\\^" } } */ diff --git a/gcc/testsuite/gcc.target/arm/interrupt-2.c b/gcc/testsuite/gcc.target/arm/interrupt-2.c index 92f8630e016b869dc6bc1b3dabaa6b14fd2cf776..289eca0f6406bb73b029b9307cdcca6e047bcfb2 100644 --- a/gcc/testsuite/gcc.target/arm/interrupt-2.c +++ b/gcc/testsuite/gcc.target/arm/interrupt-2.c @@ -1,8 +1,8 @@ /* Verify that prologue and epilogue are correct for functions with __attribute__ ((interrupt)). */ -/* { dg-do compile } */ +/* { dg-do assemble } */ /* { dg-require-effective-target arm_nothumb } */ -/* { dg-options "-O1 -marm" } */ +/* { dg-options "-O1 -marm -save-temps" } */ /* This test is not valid when -mthumb. */ extern void bar (int); @@ -16,4 +16,4 @@ void test() } /* { dg-final { scan-assembler "push\t{r0, r1, r2, r3, r4, r5, ip, lr}" } } */ -/* { dg-final { scan-assembler "pop\t{r0, r1, r2, r3, r4, r5, ip, pc}\\^" } } */ +/* { dg-final { scan-assembler "ldmfd\tsp!, {r0, r1, r2, r3, r4, r5, ip, pc}\\^" } } */ diff --git a/gcc/testsuite/gcc.target/arm/pr70830.c b/gcc/testsuite/gcc.target/arm/pr70830.c new file mode 100644 index 0000000000000000000000000000000000000000..cad903b0cf2904969355022a7850fb8381bb3ddd --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr70830.c @@ -0,0 +1,14 @@ +/* PR target/70830. */ +/* { dg-do assemble } */ +/* { dg-require-effective-target arm_arm_ok } */ +/* { dg-options "-Os -marm -save-temps" } */ + +/* This test is not valid when -mthumb. */ + +extern void prints (char *); + +void __attribute__ ((interrupt ("IRQ"))) dm3730_IRQHandler(void) +{ + prints("IRQ" ); +} +/* { dg-final { scan-assembler "ldmfd\tsp!, {r0, r1, r2, r3, ip, pc}\\^" } } */
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.