On Wed, Jan 31, 2018 at 6:35 AM, Jeff Law <l...@redhat.com> wrote: > Whee, fun, this appears to have been broken for a very long time, > possibly since the introduction of -fstack-check in 2010. Thankfully it > only affects 32 bit and only in relatively constrained circumstances. > > -fstack-check and -fstack-clash both potentially create a loop for stack > probing. In that case they both need a scratch register to hold the > loop upper bound. > > The code to allocate a scratch register first starts with the > caller-saved registers as they're zero cost. Then it'll use any callee > saved register that is actually saved. If neither is available (say > because all the caller-saved regs are used for parameter passing and > there are no callee saved register used), then the allocation routine > will push %eax on the stack and the deallocation routine will pop it off > to restore its value. > > Of course there is a *stack allocation* between those two points. So > the pop ends up restoring garbage into the register. > > Obviously the restore routine needs to use reg+d addressing to get to > the stack slot and the allocated space needs to be deallocated by the > epilogue. But sadly there's enough assertions sprinkled around that > prevent that from working as-is. > > So what this patch does is continue to use the push to allocate the > register. And it uses reg+d to restore the register after the probing > loops. The "trick" is that the space allocated by the push becomes part > of the normal stack frame after we restore the scratch register's value. > Ie, if we push a 4 byte register, then we reduce the size of the main > allocation request by 4 bytes. And everything just works. > > Clearly this code had not be exercised. So I hacked up things so that > we always generated a probing loop and always assumed that we needed to > save/restore a scratch register and enabled stack clash protections by > default. I bootstrapped that and compared testsuite runs against a run > which just had stack clash protection on by default. That did turn up > an issue, but it was with my testing hacks, not with this patch :-) > > > I (of course) also did the usual bootstrap and regression tests, using > x86_64 and i686. Hopefully this is the last iteration on the x86/x86_64 > stack clash bits :-) > > The one concern I have is do we need to tell the CFI machinery that > %eax's value was restored to its entry value?
Can you or someone that knows CFI stuff please investigate this a bit? I'm not expert in this area, and I don't feel comfortable to approve the patch that has some known loose edges in the area I don't know that well. Uros.
