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}\\^" } } */