Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
Hi Kyrill, One week went by so I've committed the change to GCC 7 as announced. Best regards, Thomas On 05/04/18 16:36, Kyrill Tkachov wrote: On 05/04/18 16:13, Thomas Preudhomme wrote: Hi Kyrill, On 04/04/18 18:20, Thomas Preudhomme wrote: Hi Kyrill, On 04/04/18 18:19, Kyrill Tkachov wrote: Hi Thomas, On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Ok, thanks for fixing this. Does this need backporting to the branches? Yes to gcc-7-branch only. The patch applies cleanly on gcc-7-branch and the same testing shows no regression. Ok to apply to gcc-7-branch once the patch has baked for 7 days in trunk? Yes, thanks. Kyrill Best regards, Thomas
Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
On 05/04/18 16:13, Thomas Preudhomme wrote: Hi Kyrill, On 04/04/18 18:20, Thomas Preudhomme wrote: Hi Kyrill, On 04/04/18 18:19, Kyrill Tkachov wrote: Hi Thomas, On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Ok, thanks for fixing this. Does this need backporting to the branches? Yes to gcc-7-branch only. The patch applies cleanly on gcc-7-branch and the same testing shows no regression. Ok to apply to gcc-7-branch once the patch has baked for 7 days in trunk? Yes, thanks. Kyrill Best regards, Thomas
Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
Hi Kyrill, On 04/04/18 18:20, Thomas Preudhomme wrote: Hi Kyrill, On 04/04/18 18:19, Kyrill Tkachov wrote: Hi Thomas, On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Ok, thanks for fixing this. Does this need backporting to the branches? Yes to gcc-7-branch only. The patch applies cleanly on gcc-7-branch and the same testing shows no regression. Ok to apply to gcc-7-branch once the patch has baked for 7 days in trunk? Best regards, Thomas
Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
Hi Kyrill, On 04/04/18 18:19, Kyrill Tkachov wrote: Hi Thomas, On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Ok, thanks for fixing this. Does this need backporting to the branches? Yes to gcc-7-branch only. Best regards, Thomas
Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
Hi Thomas, On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Ok, thanks for fixing this. Does this need backporting to the branches? Kyrill Best regards, Thomas
Re: [PATCH, GCC/ARM] Fix PR85203: cmse_nonsecure_caller returns wrong result
Oops, forgot the link. On 04/04/18 18:03, Thomas Preudhomme wrote: Hi, __builtin_cmse_nonsecure_caller implementation returns true in almost all cases due to 2 separate bugs: * gen_addsi is used instead of gen_andsi to retrieve the lsb * the lsb boolean value is not negated but the specification [1] says the intrinsic should return true for a nonsecure caller and a nonsecure caller is characterized with LR's lsb being 0 [1] https://static.docs.arm.com/ecm0359818/10/ECM0359818_armv8m_security_extensions_reqs_on_dev_tools_1_0.pdf Best regards, Thomas This was not caught due to (1) lack of runtime test and (2) the existing RTL scan not taking into account that '.' matches newline in Tcl regular expressions. This patch fixes the implementation issues and improves testing of cmse_nonsecure_caller by (1) adding a runtime test for the secure caller case and (2) looking for an SET insn of an AND expression in the right function. This leaves the nonsecure caller case only partly tested since the exact value being AND and the negation are not covered by the scan and the existing test infrastructure does not allow 2 separate compilation and link to be performed. It is enough though to catch the current incorrect behavior. The patch also reorganize the scan directives in cmse-1.c to more easily identify what function they are intended to test in the file. ChangeLog entry is as follows: *** gcc/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * config/arm/arm-builtins.c (arm_expand_builtin): Change expansion to perform a bitwise AND of the argument followed by a boolean negation of the result. *** gcc/testsuite/ChangeLog *** 2018-04-04 Thomas Preud'homme PR target/85203 * gcc.target/arm/cmse/cmse-1.c: Tighten cmse_nonsecure_caller RTL scan to match a single insn of the baz function. Move scan directives at the end of the file below the functions they are trying to test for better readability. * gcc.target/arm/cmse/cmse-16.c: New testcase. Testing: No bootstrap since only M profile builtin code has been changed but regression testing for arm-none-eabi targeting Arm Cortex-M23 and Cortex-M33 shows no regression. Is this ok for stage4? Best regards, Thomas
