vtjnash wrote: I see. On further reflection this weekend, I don't think that is the primary question to answer first. From the observation by @phyBrackets I realized the choice presented here is whether it is more important to do all optimizations and sometimes silently get ssp security wrong (the metadata approach) or more important to get ssp right (according to whatever arbitrary standard is documented about what ssp does) even if that costs llvm the ability to do some optimizations.
But also on looking into it further, I'm concerned the metadata approach is not actually feasible if we care about getting ssp "right" (for whoever defines "right"). According to my understanding of @phyBrackets concerns (https://github.com/llvm/llvm-project/pull/183623#discussion_r2863115869), it is actually semantically unacceptable to drop this metadata (either enabling ssp on a particular allocation that should not have it, or disabling ssp on a function that must have it) as the claim is that doing either would silently undermine the effectiveness of ssp. That would mean that any pass that did drop this metadata would be "wrong". But also note that accidentally dropping this metadata inherently implies activating SSP, which will cost program performance. It also means that each of the ~400 places that construct AllocaInst would need to be "fixed" as otherwise they would cause a performance impact. I cannot imagine being responsible for a change that declared nearly all existing alloca instruction creation for now being "wrong". By contrast, with this current approach, only the passes that currently analyze an alloca directly and don't use memory effects would need fixing (presumably, to actually use memory effects). I would therefore not be willing to claim that https://github.com/llvm/llvm-project/issues/66709 is fixable if we use metadata. https://github.com/llvm/llvm-project/pull/183623 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
