Re: [RFA][PATCH] Stack clash protection 06/08 - V4

[Jeff Law <law at redhat dot com>]

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