Re: [PATCH][RFA/RFC] Stack clash mitigation patch 02/08

[Jeff Law <law@redhat.com>]

On 07/12/2017 07:44 PM, Segher Boessenkool wrote:
> On Tue, Jul 11, 2017 at 03:20:12PM -0600, Jeff Law wrote:
>> 	* conifg/mips/mips.c (mips_expand_prologue): Likewise.
> 
> Typo ("conifg").
Will fix.

> 
>> --- a/gcc/defaults.h
>> +++ b/gcc/defaults.h
>> @@ -1408,8 +1408,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>>  #endif
>>  
>>  /* The default is not to move the stack pointer.  */
>> +/* The default is not to move the stack pointer, unless we are using
>> +   stack clash prevention stack checking.  */
>>  #ifndef STACK_CHECK_MOVING_SP
>> -#define STACK_CHECK_MOVING_SP 0
>> +#define STACK_CHECK_MOVING_SP\
>> +  (flag_stack_check == STACK_CLASH_BUILTIN_STACK_CHECK)
>>  #endif
> 
> Missing space before that backslash.
Similarly.

> 
> The documentation for STACK_CHECK_CONFIG_SP needs updating (its default
> is no longer zero, for one).
Yea.  Missed that.  I actually need to go back and look at this again.
I'm not entirely sure it's necessary -- it may be a relic from when I
thought more -fstack-check infrastructure was going to be reusable.


> 
> I don't really see why this is so complicated, and why the rs6000
> target changes (a later patch) are so big.  Why isn't it just simple
> patches to allocate_stack (and the prologue thing), that check the
> flag and if it is set do some probes?
Yea.  I wasn't happy with the size of the rs6000 patches either, which I
mentioned at some point :-)  Some of the complexity is making sure we
keep the backchain pointer correct and trying to do so as efficiently as
possible.  But there's too much conceptual code duplication.

Essentially the code shows up 3 times in slightly different forms.

Once when allocating space in the prologue.  Then again with the
probe_stack_range insn support, then again with the expander to allocate
dynamic stack space.

The prologue always has a known size and we should take advantage of
that, particularly since the prologue code is what needs to be the most
efficient.

The dynamic space allocation expander.  I think we should look at trying
to do less in the expander and rely more on refactor'd code from explow.

THe probe_stack_range output routine seems like it ought to be redundant
with something :-)

I'll look at this again and see if there's any good way to refactor and
simplify.

jeff