Re: [PR47785] COLLECT_AS_OPTIONS

[Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>]

On Mon, 28 Oct 2019 11:53:06 +1100
Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote:

> On Wed, 23 Oct 2019 at 23:07, Richard Biener <richard.guenther@gmail.com> wrote:

> > Did you try this with multiple assembler options?  I see you stream
> > them as -Wa,-mfpu=xyz,-mthumb but then compare the whole
> > option strings so a mismatch with -Wa,-mthumb,-mfpu=xyz would be

indeed, i'd have expected some kind of sorting, but i don't see it in
the proposed patch?

> > diagnosed.  If there's a spec induced -Wa option do we get to see
> > that as well?  I can imagine -march=xyz enabling a -Wa option
> > for example.
> >
> > +             *collect_as = XNEWVEC (char, strlen (args_text) + 1);
> > +             strcpy (*collect_as, args_text);
> >
> > there's strdup.  Btw, I'm not sure why you don't simply leave
> > the -Wa option in the merged options [individually] and match
> > them up but go the route of comparing strings and carrying that
> > along separately.  I think that would be much better.
> 
> Is attached patch which does this is OK?

> +  obstack_init (&collect_obstack);
> +  obstack_grow (&collect_obstack, "COLLECT_AS_OPTIONS=",
> +		sizeof ("COLLECT_AS_OPTIONS=") - 1);
> +  obstack_grow (&collect_obstack, "-Wa,", strlen ("-Wa,"));

Why don't you grow once, including the "-Wa," ?

> +/* Append options OPTS from -Wa, options to ARGV_OBSTACK.  */
> +
> +static void
> +append_compiler_wa_options (obstack *argv_obstack,
> +			    struct cl_decoded_option *opts,
> +			    unsigned int count)
> +{
> +  static const char *collect_as;
> +  for (unsigned int j = 1; j < count; ++j)
> +    {
> +      struct cl_decoded_option *option = &opts[j];

Instead of the switch below, why not just

if (option->opt_index != OPT_Wa_)
  continue;

here?

> +      if (j == 1)
> +	collect_as = NULL;

or at least here?

(why's collect_as static in the first place? wouldn't that live in the parent function?)

> +      const char *args_text = option->orig_option_with_args_text;
> +      switch (option->opt_index)
> +	{
> +	case OPT_Wa_:
> +	  break;
> +	default:
> +	  continue;
> +	}