uweigand wrote: The latest commit https://github.com/llvm/llvm-project/pull/169317/commits/931e6493d95dc399461e3311334fb928c11b60e2 seem to be broken?
Before reviewing the details, I still have more general questions on the approach. Combining the `LOAD_STACK_GUARD` into `MOVE_STACK_GUARD` and `COMPARE_STACK_GUARD` does make sense to me to ensure the stack canary never ends up in any register. [ As an aside, are there any uses of `LOAD_STACK_GUARD` remaining after that optimization? That would unfortunate - maybe we should rather error out if that happens? ] However, with your current approach, the *address* of the stack canary not only end up in a register (this seems to be unavoidable), but it might actually end up spilled to a *stack slot* - the `LOAD_STACK_GUARD_ADDRESS` is created before register allocation, so it loads the address into a virtual register that might get spilled later. [ Note that given that fact, it seems pointless to move the implementation of loading the address all the way to the AsmPrinter. ] Having checked some of the history of the stack protector implementation in GCC, it does appear that having the address of the stack canary spilled to the stack is a potential weakness that should be avoided. Given that, this new implementation is even *worse* in this respect then the current implementation - the current implementation does leak the canary into a register, but at least neither the canary nor its address can end up spilled to the stack. I seem to recall that in an earlier attempt, you delayed the split between loading and using the address to post-RA, but that ran into register allocation problems with the scratch register? I think we might have to go back to that approach and fix those register allocation problems - sorry! https://github.com/llvm/llvm-project/pull/169317 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
