Good thing I did, found a missing earlyclobber in the process. Rerunning all tests again.
Best regards, Thomas On Wed, 24 Oct 2018 at 10:13, Thomas Preudhomme <thomas.preudho...@linaro.org> wrote: > > Please hold on for the reviews, found a small improvement that could > be done. Am testing it right now, should have something by tonight or > tomorrow. > > Best regards, > > Thomas > On Tue, 23 Oct 2018 at 13:35, Thomas Preudhomme > <thomas.preudho...@linaro.org> wrote: > > > > [Removing Jeff Law since middle end code hasn't changed] > > > > Hi, > > > > Given how memory operand are reloaded even with an X constraint, I've > > reworked the patch for the combined set and combined test instruction > > ot keep the mem out of the match_operand and used an expander to > > generate the right instruction pattern. I've also fixed some > > longstanding issues with the patch when flag_pic is true and with > > constraints for Thumb-1 that I hadn't noticed before due to using > > dg-cmp-results in conjunction with test_summary which does not show > > NA->FAIL (see [1]). > > > > All in all, I think the Arm code would do with a fresh review rather > > than looking at the changes since last posted version. (unchanged) > > ChangeLog entries are as follows: > > > > *** gcc/ChangeLog *** > > > > 2018-08-09 Thomas Preud'homme <thomas.preudho...@linaro.org> > > > > * target-insns.def (stack_protect_combined_set): Define new standard > > pattern name. > > (stack_protect_combined_test): Likewise. > > * cfgexpand.c (stack_protect_prologue): Try new > > stack_protect_combined_set pattern first. > > * function.c (stack_protect_epilogue): Try new > > stack_protect_combined_test pattern first. > > * config/arm/arm.c (require_pic_register): Add pic_reg and compute_now > > parameters to control which register to use as PIC register and force > > reloading PIC register respectively. Insert in the stream of insns if > > possible. > > (legitimize_pic_address): Expose above new parameters in prototype and > > adapt recursive calls accordingly. Use pic_reg if non null instead of > > cached one. > > (arm_load_pic_register): Add pic_reg parameter and use it if non null. > > (arm_legitimize_address): Adapt to new legitimize_pic_address > > prototype. > > (thumb_legitimize_address): Likewise. > > (arm_emit_call_insn): Adapt to require_pic_register prototype change. > > (arm_expand_prologue): Adapt to arm_load_pic_register prototype change. > > (thumb1_expand_prologue): Likewise. > > * config/arm/arm-protos.h (legitimize_pic_address): Adapt to prototype > > change. > > (arm_load_pic_register): Likewise. > > * config/arm/predicated.md (guard_addr_operand): New predicate. > > (guard_operand): New predicate. > > * config/arm/arm.md (movsi expander): Adapt to legitimize_pic_address > > prototype change. > > (builtin_setjmp_receiver expander): Adapt to thumb1_expand_prologue > > prototype change. > > (stack_protect_combined_set): New expander.. > > (stack_protect_combined_set_insn): New insn_and_split pattern. > > (stack_protect_set_insn): New insn pattern. > > (stack_protect_combined_test): New expander. > > (stack_protect_combined_test_insn): New insn_and_split pattern. > > (stack_protect_test_insn): New insn pattern. > > * config/arm/unspecs.md (UNSPEC_SP_SET): New unspec. > > (UNSPEC_SP_TEST): Likewise. > > * doc/md.texi (stack_protect_combined_set): Document new standard > > pattern name. > > (stack_protect_set): Clarify that the operand for guard's address is > > legal. > > (stack_protect_combined_test): Document new standard pattern name. > > (stack_protect_test): Clarify that the operand for guard's address is > > legal. > > > > *** gcc/testsuite/ChangeLog *** > > > > 2018-07-05 Thomas Preud'homme <thomas.preudho...@linaro.org> > > > > * gcc.target/arm/pr85434.c: New test. > > > > Testing: Bootstrap and regression testing for Arm, Thumb-1 and Thumb-2 > > with (i) default flags, (ii) an extra -fstack-protect-all and (iii) > > -fPIC -fstack-protect-all. A glibc build and testsuite run was also > > performed for Arm and Thumb-2. Default flags show no regression and > > the other runs have some expected scan-assembler failing (due to stack > > protector or fPIC code sequence), as well as guality fail (due to less > > optimized code with the new stack protector code) and some execution > > failures in sibcall-9 and sibcall-10 under -fPIC -fstack-protector-all > > due to the PIC sequence for the global variable making the frame > > layout different for the 2 functions (these become PASS if making the > > global variable static). > > > > Is this ok for trunk? > > > > Best regards, > > > > Thomas > > > > [1] https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01412.html > > > > > > On Tue, 25 Sep 2018 at 17:10, Kyrill Tkachov > > <kyrylo.tkac...@foss.arm.com> wrote: > > > > > > Hi Thomas, > > > > > > On 29/08/18 10:51, Thomas Preudhomme wrote: > > > > Resend hopefully without HTML this time. > > > > > > > > On Wed, 29 Aug 2018 at 10:49, Thomas Preudhomme > > > > <thomas.preudho...@linaro.org> wrote: > > > >> Hi, > > > >> > > > >> I've reworked the patch fixing PR85434 (spilling of stack protector > > > >> guard's address on ARM) to address the testsuite regression on powerpc > > > >> and x86 as well as glibc testsuite regression on ARM. Issues were due > > > >> to unconditionally attempting to generate the new patterns. The code > > > >> now tests if there is a pattern for them for the target before > > > >> generating them. In the ARM side of the patch, I've also added a more > > > >> specific predicate for the new patterns. The new patch is found below. > > > >> > > > >> > > > >> In case of high register pressure in PIC mode, address of the stack > > > >> protector's guard can be spilled on ARM targets as shown in PR85434, > > > >> thus allowing an attacker to control what the canary would be compared > > > >> against. ARM does lack stack_protect_set and stack_protect_test insn > > > >> patterns, defining them does not help as the address is expanded > > > >> regularly and the patterns only deal with the copy and test of the > > > >> guard with the canary. > > > >> > > > >> This problem does not occur for x86 targets because the PIC access and > > > >> the test can be done in the same instruction. Aarch64 is exempt too > > > >> because PIC access insn pattern are mov of UNSPEC which prevents it > > > >> from > > > >> the second access in the epilogue being CSEd in cse_local pass with the > > > >> first access in the prologue. > > > >> > > > >> The approach followed here is to create new "combined" set and test > > > >> standard pattern names that take the unexpanded guard and do the set or > > > >> test. This allows the target to use an opaque pattern (eg. using > > > >> UNSPEC) > > > >> to hide the individual instructions being generated to the compiler and > > > >> split the pattern into generic load, compare and branch instruction > > > >> after register allocator, therefore avoiding any spilling. This is here > > > >> implemented for the ARM targets. For targets not implementing these new > > > >> standard pattern names, the existing stack_protect_set and > > > >> stack_protect_test pattern names are used. > > > >> > > > >> To be able to split PIC access after register allocation, the functions > > > >> had to be augmented to force a new PIC register load and to control > > > >> which register it loads into. This is because sharing the PIC register > > > >> between prologue and epilogue could lead to spilling due to CSE again > > > >> which an attacker could use to control what the canary gets compared > > > >> against. > > > >> > > > >> ChangeLog entries are as follows: > > > >> > > > >> *** gcc/ChangeLog *** > > > >> > > > >> 2018-08-09 Thomas Preud'homme <thomas.preudho...@linaro.org> > > > >> > > > >> * target-insns.def (stack_protect_combined_set): Define new > > > >> standard > > > >> pattern name. > > > >> (stack_protect_combined_test): Likewise. > > > >> * cfgexpand.c (stack_protect_prologue): Try new > > > >> stack_protect_combined_set pattern first. > > > >> * function.c (stack_protect_epilogue): Try new > > > >> stack_protect_combined_test pattern first. > > > >> * config/arm/arm.c (require_pic_register): Add pic_reg and > > > >> compute_now > > > >> parameters to control which register to use as PIC register and > > > >> force > > > >> reloading PIC register respectively. Insert in the stream of > > > >> insns if > > > >> possible. > > > >> (legitimize_pic_address): Expose above new parameters in > > > >> prototype and > > > >> adapt recursive calls accordingly. > > > >> (arm_legitimize_address): Adapt to new legitimize_pic_address > > > >> prototype. > > > >> (thumb_legitimize_address): Likewise. > > > >> (arm_emit_call_insn): Adapt to new require_pic_register prototype. > > > >> * config/arm/arm-protos.h (legitimize_pic_address): Adapt to > > > >> prototype > > > >> change. > > > >> * config/arm/predicated.md (guard_operand): New predicate. > > > > > > Typo, predicates.md is the filename. > > > > > > Looks ok to me otherwise. > > > Thank you for your patience. > > > > > > Kyrill > > > > > > >> * config/arm/arm.md (movsi expander): Adapt to > > > >> legitimize_pic_address > > > >> prototype change. > > > >> (stack_protect_combined_set): New insn_and_split pattern. > > > >> (stack_protect_set): New insn pattern. > > > >> (stack_protect_combined_test): New insn_and_split pattern. > > > >> (stack_protect_test): New insn pattern. > > > >> * config/arm/unspecs.md (UNSPEC_SP_SET): New unspec. > > > >> (UNSPEC_SP_TEST): Likewise. > > > >> * doc/md.texi (stack_protect_combined_set): Document new standard > > > >> pattern name. > > > >> (stack_protect_set): Clarify that the operand for guard's address > > > >> is > > > >> legal. > > > >> (stack_protect_combined_test): Document new standard pattern name. > > > >> (stack_protect_test): Clarify that the operand for guard's > > > >> address is > > > >> legal. > > > >> > > > >> *** gcc/testsuite/ChangeLog *** > > > >> > > > >> 2018-07-05 Thomas Preud'homme <thomas.preudho...@linaro.org> > > > >> > > > >> * gcc.target/arm/pr85434.c: New test. > > > > > > >> > > > >> Testing: > > > >> > > > >> native x86_64: bootstrap + testsuite -> no regression, can see > > > >> failures with previous version of patch but not with new version > > > >> native powerpc64: bootstrap + testsuite -> no regression, can see > > > >> failures from pr86834 with previous version of patch but not with new > > > >> version > > > >> cross ARM Linux: build + testsuite -> no regression > > > >> native ARM Thumb-2: bootstrap + testsuite + glibc build + glibc test > > > >> -> no regression > > > >> native ARM Arm: bootstrap + testsuite + glibc build + glibc test -> no > > > >> regression > > > >> Aarch64: bootstrap + testsuite + glibc build + glibc test-> no > > > >> regression > > > >> > > > >> Is this ok for trunk? > > > >> > > > >> Best regards, > > > >> > > > >> Thomas > > >
