Re: [PATCH v5] add -fprolog-pad=N,M option

[Torsten Duwe <duwe@suse.de>]

On Wed, Feb 15, 2017 at 11:01:16AM +0000, Richard Earnshaw (lists) wrote:
> On 13/01/17 12:19, Torsten Duwe wrote:
> 
> > +++ b/gcc/doc/invoke.texi
> > @@ -11341,6 +11341,27 @@ of the function name, it is considered to be a match.  For C99 and C++
> >  extended identifiers, the function name must be given in UTF-8, not
> >  using universal character names.
> >  
> > +@item -fprolog-pad=@var{N},@var{M}
> This needs to make it clear that M is optional.  Then below state that
> if omitted, M defaults to zero.

It was mentioned, further down in the paragraph. I moved it up.

> > --- a/gcc/opts.c
> > +++ b/gcc/opts.c
> > @@ -2157,6 +2157,26 @@ common_handle_option (struct gcc_options *opts,
> >          opts->x_flag_ipa_reference = false;
> >        break;
> >  
> > +    case OPT_fprolog_pad_:
> > +      {
> > +	const char *comma = strchr (arg, ',');
> > +	if (comma)
> > +	  {
> > +	    prolog_nop_pad_size = atoi (arg);
> > +	    prolog_nop_pad_entry = atoi (comma + 1);
> > +	  }
> > +	else
> > +	  {
> > +	    prolog_nop_pad_size = atoi (arg);
> > +	    prolog_nop_pad_entry = 0;
> > +	  }
> 
> Where's the error checking?  If I write gibberish after the option name
> then atoi will silently fail and return zero.  I'm not overly familiar
> with the option handling code, but I'm sure we have routines to do the
> heavy lifting here.

Yes, I had already found integral_argument, but that's unsuitable for a
comma separated list, and arg is const so I could' punch a \0 there.
Using atoi was just lazy, admittedly.

> > +default_print_prolog_pad (FILE *file, unsigned HOST_WIDE_INT pad_size,
> > +			  bool record_p)
> > +{
> > +  if (record_p)
> > +    fprintf (file, "1:");
> > +
> > +  unsigned i;
> > +  for (i = 0; i < pad_size; ++i)
> > +    fprintf (file, "\tnop\n");
> > +
> > +  if (record_p)
> > +    {
> > +      fprintf (file, "\t.section __prolog_pads_loc, \"a\",@progbits\n");
> > +      fprintf (file, "\t.quad 1b\n");
> > +      fprintf (file, "\t.previous\n");
> > +    }
> > +}
> 
> NO!  Almost everything in this function is wrong, it needs to be done
> through suitable hooks that call into the machine back-ends that
> understand assembly flavours supported.

That was already mentioned in a previous version. That code assumes GAS+ELF.
It was the quick and dirty solution to get a working prototype.

	Torsten