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 > >
