dominik-steenken wrote: > The latest commit > [931e649](https://github.com/llvm/llvm-project/commit/931e6493d95dc399461e3311334fb928c11b60e2) > seem to be broken?
That is quite strange, debugging it now. > 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? ] I don't think that uses of `LOAD_STACK_GUARD` would remain. We could assert out in the asm printer if we encounter one instead of trying to properly expand it. > 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. ] Could we not use the same approach that the existing `LOAD_STACK_GUARD` uses in order to avoid the address from being spilled? I.e. could we not mark `LOAD_STACK_GUARD_ADDRESS` `isReMaterializable`, and do some further nudging with kill flags to ensure that the value isn't reused? Defining it as `mayLoad` and adding a volatile `MachineMemOperand` might also, help, right? Although it is not technically a load in all cases, and `mayLoad = 1` also adds some code paths that assume that there is an operand attached to the instruction that is loaded "from", so there may be additional work there. > 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 understand but i am hoping that this can be avoided with the same approach that current code uses to avoid leaking the canary to the stack (see above). > 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! I did have issues with scratch registers, yes. From what i'm reading, i would have to use the RegisterScavenger to obtain an unused register, and I was having some trouble with that. I guess that could be taken up again. https://github.com/llvm/llvm-project/pull/169317 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
