Re: [PATCH][RFA/RFC] Stack clash mitigation patch 01/08 - V3

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2017-08-22 at 09:35 -0600, Jeff Law wrote:
> On 08/19/2017 12:22 PM, Martin Sebor wrote:
> > On 07/30/2017 11:35 PM, Jeff Law wrote:
> > > This patch introduces the stack clash protection options
> > > 
> > > Changes since V2:
> > > 
> > > Adds two new params.  The first controls the size of the guard
> > > area.
> > > This controls the threshold for when a function prologue requires
> > > probes.  The second controls the probing interval -- ie, once
> > > probes are
> > > needed, how often do we emit them.  These are really meant more
> > > for
> > > developers to experiment with than users.  Regardless I did go
> > > ahead and
> > > document them./PARAM
> > > 
> > > It also adds some sanity checking WRT combining stack clash
> > > protection
> > > with -fstack-check.
> > 
> > Just a minor nit and suggestion:
> > 
> > "supproted" -> "supported"
> > 
> > +      warning_at (UNKNOWN_LOCATION, 0,
> > +          "-fstack-clash_protection is not supproted on targets "
> > +          "where the stack grows from lower to higher addresses");
> > 
> > and quote the name of the options in diagnostics, i.e., use either
> > 
> >   "%<"-fstack-clash_protection%> ..."
> > 
> > or
> > 
> >   "%qs is not supported...", "-fstack-clash_protection"
> > 
> > as you did in error ("value of parameter %qs must be a power of 2",
> > ompiler_params[i].option);
> > 
> > Likewise in
> > 
> > +      warning_at (UNKNOWN_LOCATION, 0,
> > +          "-fstack-check= and -fstack-clash_protection are
> > mutually "
> > +          "exclusive.  Disabling -fstack-check=");
> 
> Thanks.  I settled on the %< %> style.  None of the other warnings in
> that area use either.  Otherwise I would have just selected whatever
> was
> most commonly used in that code.

A nit that don't seem to have been mentioned: the patch refers to
  -fstack-clash_protection (erroneous underscore)
in two places, which should be:
  -fstack-clash-protection (all dashes)


Dave