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?
> >> + 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?)
(You could still do
if (!size_rtx)
size_rtx = GEN_INT (-size_int);
to simplify the callers a bit).
> > (These comments were in the original code already, oh well).
> Happy to adjust if you've got a suggestion on how to make it clearer.
I'm drawing a blank right now :-/
> >> +rs6000_emit_probe_stack_range_stack_clash (HOST_WIDE_INT orig_size,
> >> + rtx copy_reg)
> >> +{
> >> + rtx orig_sp = copy_reg;
> >> +
> >> + HOST_WIDE_INT probe_interval
> >> + = 1 << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
> >
> > HOST_WIDE_INT_1U << ...
> It won't matter in practice because of the limits we've put on those
> PARAMs, but sure, easy to do except for the long lines, even in a helper
> function :(
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) :-)
> >> +/* Probe a range of stack addresses from REG1 to REG3 inclusive. These
> >> are
> >> + absolute addresses. REG2 contains the backchain that must be stored
> >> into
> >> + *sp at each allocation.
> >
> > I would just remove "These are absolute addresses.", or write something
> > like "These are addresses, not offsets", but that is kind of obvious
> > isn't it ;-)
> :-) It was copied from the analogous -fstack-check routine. Note
> there's a similar routine for -fstack-check which uses offsets, so I
> think being very explicit makes sense. Perhaps just "These are
> addresses, not offsets" is better than the current comment? I'll go
> with whatever you prefer here.
Yeah that is fine, thanks.
> >> +static const char *
> >> +output_probe_stack_range_stack_clash (rtx reg1, rtx reg2, rtx reg3)
> >> +{
> >> + static int labelno = 0;
> >> + char loop_lab[32];
> >> + rtx xops[3];
> >> +
> >> + HOST_WIDE_INT probe_interval
> >> + = 1 << PARAM_VALUE (PARAM_STACK_CLASH_PROTECTION_PROBE_INTERVAL);
> >
> > Once more :-) Maybe a helper function is in order? Would avoid the
> > huge length names at least.
> 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).
But we'll probably need to support 4kB as well, even for 64-bit.
> > 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!
Segher
