Re: [PR47785] COLLECT_AS_OPTIONS

[Kugan Vivekanandarajah <kugan dot vivekanandarajah at linaro dot org>]

Hi Bernhard,

Thanks for the review.

On Tue, 29 Oct 2019 at 08:52, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
>
> 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?
Let me  try to see what is the best way to handle this. If we look at
Richard Earnshaw's comment in the next email, there are cases where
handling this would not be straightforward. I am happy to do what is
acceptable  here.


>
> > > 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," ?

I will change this.
>
> > +/* 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;

I will change this.

>
> 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?)
I am keeping the -Wa options which come from last TU here and making
sure they are the same. If we get -Wa options with different
incompatible options, handling them is tricky. So in this patch I want
to handle only when they are the same and flag error otherwise. It
again goes back to your first comment. I am happy to workout what is
an acceptable  solution here.

Thanks,
Kugan

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