On Mon, 28 Oct 2019 11:53:06 +1100
Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote:
> On Wed, 23 Oct 2019 at 23:07, Richard Biener <richard.guent...@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;
> + }
