On 09/25/2017 11:25 AM, Segher Boessenkool wrote: > On Mon, Sep 25, 2017 at 10:00:55AM -0600, Jeff Law wrote: >> On 09/25/2017 04:52 AM, Segher Boessenkool wrote: >>> Did you also test Ada? It needs testing. I wanted to try it myself, >>> but your patch doesn't apply (you included the changelog bits in the >>> patch), and I cannot easily manually apply it either because you sent >>> it as base64 instead of as text. >> I didn't test Ada with -fstack-clash-protection on by default. I did >> test it as part of the normal bootstrap & regression test cycle with no >> changes in the Ada testsuites. >> >> Testing it with stack clash protection on by default is easy to do :-) >> I wouldn't be surprised to see failures since some tests are known to >> test/use -fstack-check= which explicitly conflicts with stack clash >> protection. > > Right, and that did happen (see other mails). But that's okay, it won't > be enabled by default (yet) (right?), and when it does just the testcases > need fixing? Correct, it's not enabled by default at this time. To propose that we'd need to do some testsuite work. We'd also need to make some decisions on how to handle the partially protected targets.
So for the immediate future, stack clash protection has to be explicitly requested. We're still working out the long term plan for RHEL, but defaulting it on for all the RHEL architectures is definitely part of that plan. > >>>> + SIZE_INT is used to create the CFI note for the allocation. >>>> + >>>> + SIZE_RTX is an rtx containing the size of the adjustment. Note that >>>> + since stacks grow to lower addresses its runtime value is -SIZE_INT. >>> >>> The size_rtx doesn't always correspond to size_int so this a bit >>> misleading. >> The value at runtime of SIZE_RTX is always -SIZE_INT. It might be held >> in a register, but that register's value is always -SIZE_INT. > > Ah, I was looking at the last call in rs6000_emit_allocate_stack. It's > a bit hard to follow, but I see you are right. Unfortunate that you > won't get good generated code if you just drop that size_rtx parameter > and generate it from size_int here (or do you?) I don't think it would make any difference from a code generation standpoint. One could argue that passing in SIZE_RTX is just silly as the caller's don't really need that information and it just hides the invariant that SIZE_RTX is just -SIZE at runtime. Let me give that a whirl in the tester. > > Yeah. But whenever I see "1 <<" I think "will it fit" -- no need for > that with HOST_WIDE_INT_1 (or unsigned, if you can) :-) And that's why I just went ahead and wrote a helper. It ensures we're good to go even if we expand the limits significantly. >> Fixed. Long term I hope we find that changing these things isn't useful >> and we just drop them. > > I hope we can change to 64kB for 64-bit Power (instead of 4kB) -- if we > do, we can probably turn on this protection by default, since the runtime > cost will be close to zero (almost all functions will need *no* extra > code compared to no clash protection). Right. But I would claim that even today with 4k guard and 4k probe interval that the overhead is likely not measurable in practice on a target like ppc. > > But we'll probably need to support 4kB as well, even for 64-bit. Note that we can adjust the size of the guard and probe interval independently. So if a system is providing a multi-page guard we can take advantage of that by raising the former, but not the latter. > >>> Bootstrap+testsuite finished on BE, but I forgot to enable stack-clash >>> protection by default, whoops. Will have results later today (also LE). >> FWIW, I did do BE tests of earlier versions of this code as well as >> ppc32-be with and without stack clash protection enabled by default. >> But most of the time I'm testing ppc64le. > > So all testing was fine (except the things with stack clash protection > on by default, which you are not proposing to commit). > > I'm quite happy with the patch now; it's okay for trunk. > > Thank you for all the work! Thanks. Your feedback on both the PPC and generic bits did significantly improve the implementation. Jeff
