| Message ID | d21d98cc-1f24-599a-116c-3a9f36928a55@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [GCC/ARM] Fix cmse_nonsecure_entry return insn size | expand |
Hi Thomas, On 08/11/17 09:50, Thomas Preudhomme wrote: > Hi, > > A number of instructions are output in assembler form by > output_return_instruction () when compiling a function with the > cmse_nonsecure_entry attribute for Armv8-M Mainline with hardfloat float > ABI. However, the corresponding thumb2_cmse_entry_return insn pattern > does not account for all these instructions in its computing of the > length of the instruction. > > This may lead GCC to use the wrong branching instruction due to > incorrect computation of the offset between the branch instruction's > address and the target address. > > This commit fixes the mismatch between what output_return_instruction () > does and what the pattern think it does and adds a note warning about > mismatch in the affected functions' heading comments to ensure code does > not get out of sync again. > > Note: no test is provided because the C testcase is fragile (only works > on GCC 6) and the extracted RTL test fails to compile due to bugs in the > RTL frontend (PR82815 and PR82817) > > ChangeLog entries are as follows: > > *** gcc/ChangeLog *** > > 2017-10-30 Thomas Preud'homme <thomas.preudhomme@arm.com> > > * config/arm/arm.c (output_return_instruction): Add comments to > indicate requirement for cmse_nonsecure_entry return to account > for the size of clearing instruction output here. > (thumb_exit): Likewise. > * config/arm/thumb2.md (thumb2_cmse_entry_return): Fix length for > return in hardfloat mode. > > Testing: Bootstrapped on arm-linux-gnueabihf and testsuite shows no > regression. > > Is this ok for trunk? > Ok for trunk and for the branches after a few days. Thanks, Kyrill > Best regards, > > Thomas
Hi Kyrill, On 09/11/17 14:26, Kyrill Tkachov wrote: > Hi Thomas, > > On 08/11/17 09:50, Thomas Preudhomme wrote: >> Hi, >> >> A number of instructions are output in assembler form by >> output_return_instruction () when compiling a function with the >> cmse_nonsecure_entry attribute for Armv8-M Mainline with hardfloat float >> ABI. However, the corresponding thumb2_cmse_entry_return insn pattern >> does not account for all these instructions in its computing of the >> length of the instruction. >> >> This may lead GCC to use the wrong branching instruction due to >> incorrect computation of the offset between the branch instruction's >> address and the target address. >> >> This commit fixes the mismatch between what output_return_instruction () >> does and what the pattern think it does and adds a note warning about >> mismatch in the affected functions' heading comments to ensure code does >> not get out of sync again. >> >> Note: no test is provided because the C testcase is fragile (only works >> on GCC 6) and the extracted RTL test fails to compile due to bugs in the >> RTL frontend (PR82815 and PR82817) >> >> ChangeLog entries are as follows: >> >> *** gcc/ChangeLog *** >> >> 2017-10-30 Thomas Preud'homme <thomas.preudhomme@arm.com> >> >> * config/arm/arm.c (output_return_instruction): Add comments to >> indicate requirement for cmse_nonsecure_entry return to account >> for the size of clearing instruction output here. >> (thumb_exit): Likewise. >> * config/arm/thumb2.md (thumb2_cmse_entry_return): Fix length for >> return in hardfloat mode. >> >> Testing: Bootstrapped on arm-linux-gnueabihf and testsuite shows no >> regression. >> >> Is this ok for trunk? >> > > Ok for trunk and for the branches after a few days. I've committed the patch to gcc-7-branch (see attached) after another round of testing since nobody reported a regression since. Thanks. Best regards, Thomas diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 989957f048e3c757ef4665d0387ecdc66d26a7dd..7b3f4c1011dc37cb01654f70cfbffadd57d382ec 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -19316,7 +19316,12 @@ arm_get_vfp_saved_size (void) /* Generate a function exit sequence. If REALLY_RETURN is false, then do everything bar the final return instruction. If simple_return is true, - then do not output epilogue, because it has already been emitted in RTL. */ + then do not output epilogue, because it has already been emitted in RTL. + + Note: do not forget to update length attribute of corresponding insn pattern + when changing assembly output (eg. length attribute of + thumb2_cmse_entry_return when updating Armv8-M Mainline Security Extensions + register clearing sequences). */ const char * output_return_instruction (rtx operand, bool really_return, bool reverse, bool simple_return) @@ -23809,7 +23814,12 @@ thumb_pop (FILE *f, unsigned long mask) /* Generate code to return from a thumb function. If 'reg_containing_return_addr' is -1, then the return address is - actually on the stack, at the stack pointer. */ + actually on the stack, at the stack pointer. + + Note: do not forget to update length attribute of corresponding insn pattern + when changing assembly output (eg. length attribute of epilogue_insns when + updating Armv8-M Baseline Security Extensions register clearing + sequences). */ static void thumb_exit (FILE *f, int reg_containing_return_addr) { diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md index 2e7580f220eae1524fef69719b1796f50f5cf27c..35f8e9bbf24058c129cbb117c74d1a4bebbf9f38 100644 --- a/gcc/config/arm/thumb2.md +++ b/gcc/config/arm/thumb2.md @@ -1132,7 +1132,7 @@ ; we adapt the length accordingly. (set (attr "length") (if_then_else (match_test "TARGET_HARD_FLOAT") - (const_int 12) + (const_int 34) (const_int 8))) ; We do not support predicate execution of returns from cmse_nonsecure_entry ; functions because we need to clear the APSR. Since predicable has to be
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 033ec255a577f782201527f57f45802bc0eb45e0..9919f54242d9317125a104f9777d76a85de80e9b 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -19417,7 +19417,12 @@ arm_get_vfp_saved_size (void) /* Generate a function exit sequence. If REALLY_RETURN is false, then do everything bar the final return instruction. If simple_return is true, - then do not output epilogue, because it has already been emitted in RTL. */ + then do not output epilogue, because it has already been emitted in RTL. + + Note: do not forget to update length attribute of corresponding insn pattern + when changing assembly output (eg. length attribute of + thumb2_cmse_entry_return when updating Armv8-M Mainline Security Extensions + register clearing sequences). */ const char * output_return_instruction (rtx operand, bool really_return, bool reverse, bool simple_return) @@ -23950,7 +23955,12 @@ thumb_pop (FILE *f, unsigned long mask) /* Generate code to return from a thumb function. If 'reg_containing_return_addr' is -1, then the return address is - actually on the stack, at the stack pointer. */ + actually on the stack, at the stack pointer. + + Note: do not forget to update length attribute of corresponding insn pattern + when changing assembly output (eg. length attribute of epilogue_insns when + updating Armv8-M Baseline Security Extensions register clearing + sequences). */ static void thumb_exit (FILE *f, int reg_containing_return_addr) { diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md index b78c3d256aeafc2eeb3dcdc2b9b07b1af9df5294..776d611d2538e790a5f504995050ffdfc51d7193 100644 --- a/gcc/config/arm/thumb2.md +++ b/gcc/config/arm/thumb2.md @@ -1132,7 +1132,7 @@ ; we adapt the length accordingly. (set (attr "length") (if_then_else (match_test "TARGET_HARD_FLOAT") - (const_int 12) + (const_int 34) (const_int 8))) ; We do not support predicate execution of returns from cmse_nonsecure_entry ; functions because we need to clear the APSR. Since predicable has to be