On 09/25/2017 04:52 AM, Segher Boessenkool wrote:
>
>> I also flipped things so that clash protection is enabled by default and
>> re-ran the tests. The idea being to see if I could exercise the path
>> that uses SP_ADJUST a bit more. But that gave me the same results.
>> While I think the change in the return value in
>> rs6000_emit_probe_stack_range_stack_clash is more correct, I don't have
>> a good way to test it.
>
> 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.
>
> (... Okay, I think I have it working; testing now).
>
> Some comments, mostly trivial comment stuff:
>
>
>> +/* Allocate SIZE_INT bytes on the stack using a store with update style insn
>> + and set the appropriate attributes for the generated insn. Return the
>> + generated insn.
>
> "Return the first generated insn"?
Fixed.
>> + 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.
>
> (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.
>
>> + COPY_REG, if non-null, should contain a copy of the original
>> + stack pointer at exit from this function.
>
> "Return a copy of the original stack pointer in COPY_REG if that is
> non-null"? It wasn't clear to me that it is this function that should
> set it :-)
It's not clear to me either. I'm just trying to preserve behavior of
the existing code in its handling of COPY_REG and COPY_OFF.
>
>> +static rtx_insn *
>> +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 :(
>
>> + /* If explicitly requested,
>> + or the rounded size is not the same as the original size
>> + or the the rounded size is greater than a page,
>> + then we will need a copy of the original stack pointer. */
>> + if (rounded_size != orig_size
>> + || rounded_size > probe_interval
>> + || copy_reg)
>> + {
>> + /* If the caller requested a copy of the incoming stack pointer,
>> + then ORIG_SP == COPY_REG and will not be NULL.
>> +
>> + If no copy was requested, then we use r0 to hold the copy. */
>> + if (orig_sp == NULL_RTX)
>> + orig_sp = gen_rtx_REG (Pmode, 0);
>> + emit_move_insn (orig_sp, stack_pointer_rtx);
>
> Maybe just write the "if" as "if (!copy_reg)"? You can lose the first
> half of the comment then (since it is obvious then).
Agreed.
>
>> + for (int i = 0; i < rounded_size; i += probe_interval)
>> + {
>> + rtx_insn *insn
>> + = rs6000_emit_allocate_stack_1 (probe_interval,
>> + probe_int, orig_sp);
>> + if (!retval)
>> + retval = insn;
>> + }
>
> Maybe "if (i == 0)" is clearer?
No strong opinions here. I added a comment and changed to i == 0
>
>> @@ -25509,6 +25703,23 @@ rs6000_emit_allocate_stack (HOST_WIDE_INT size, rtx
>> copy_reg, int copy_off)
>> warning (0, "stack limit expression is not supported");
>> }
>>
>> + if (flag_stack_clash_protection)
>> + {
>> + if (size < (1 << PARAM_VALUE
>> (PARAM_STACK_CLASH_PROTECTION_GUARD_SIZE)))
>
> HOST_WIDE_INT_1U again.
Fixed.
>
>> +/* 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.
>
>> +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.
>
>
> 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.
jeff
