On Thu, Feb 6, 2020 at 1:48 PM Prathamesh Kulkarni
<[email protected]> wrote:
>
> On Tue, 4 Feb 2020 at 19:44, Richard Biener <[email protected]>
> wrote:
> >
> > On Mon, Feb 3, 2020 at 12:37 PM Prathamesh Kulkarni
> > <[email protected]> wrote:
> > >
> > > On Thu, 30 Jan 2020 at 19:10, Richard Biener <[email protected]>
> > > wrote:
> > > >
> > > > On Thu, Jan 30, 2020 at 5:31 AM Prathamesh Kulkarni
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, 28 Jan 2020 at 17:17, Richard Biener
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Fri, Jan 24, 2020 at 7:04 AM Prathamesh Kulkarni
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, 20 Jan 2020 at 15:44, Richard Biener
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jan 8, 2020 at 11:20 AM Prathamesh Kulkarni
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, 5 Nov 2019 at 17:38, Richard Biener
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 5, 2019 at 12:17 AM Kugan Vivekanandarajah
> > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > > > > Thanks for the review.
> > > > > > > > > > >
> > > > > > > > > > > On Tue, 5 Nov 2019 at 03:57, H.J. Lu
> > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Sun, Nov 3, 2019 at 6:45 PM Kugan Vivekanandarajah
> > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for the reviews.
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Sat, 2 Nov 2019 at 02:49, H.J. Lu
> > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Oct 31, 2019 at 6:33 PM Kugan
> > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, 30 Oct 2019 at 03:11, H.J. Lu
> > > > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Sun, Oct 27, 2019 at 6:33 PM Kugan
> > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Wed, 23 Oct 2019 at 23:07, Richard Biener
> > > > > > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Mon, Oct 21, 2019 at 10:04 AM Kugan
> > > > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Thanks for the pointers.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > On Fri, 11 Oct 2019 at 22:33, Richard
> > > > > > > > > > > > > > > > > > > Biener <[email protected]> wrote:
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > On Fri, Oct 11, 2019 at 6:15 AM Kugan
> > > > > > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > On Wed, 2 Oct 2019 at 20:41, Richard
> > > > > > > > > > > > > > > > > > > > > Biener <[email protected]>
> > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > On Wed, Oct 2, 2019 at 10:39 AM
> > > > > > > > > > > > > > > > > > > > > > Kugan Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > > > <[email protected]>
> > > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > As mentioned in the PR, attached
> > > > > > > > > > > > > > > > > > > > > > > patch adds COLLECT_AS_OPTIONS for
> > > > > > > > > > > > > > > > > > > > > > > passing assembler options
> > > > > > > > > > > > > > > > > > > > > > > specified with -Wa, to the
> > > > > > > > > > > > > > > > > > > > > > > link-time driver.
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > The proposed solution only works
> > > > > > > > > > > > > > > > > > > > > > > for uniform -Wa options across all
> > > > > > > > > > > > > > > > > > > > > > > TUs. As mentioned by Richard
> > > > > > > > > > > > > > > > > > > > > > > Biener, supporting non-uniform
> > > > > > > > > > > > > > > > > > > > > > > -Wa flags
> > > > > > > > > > > > > > > > > > > > > > > would require either adjusting
> > > > > > > > > > > > > > > > > > > > > > > partitioning according to flags or
> > > > > > > > > > > > > > > > > > > > > > > emitting multiple object files
> > > > > > > > > > > > > > > > > > > > > > > from a single LTRANS CU. We could
> > > > > > > > > > > > > > > > > > > > > > > consider this as a follow up.
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > Bootstrapped and regression tests
> > > > > > > > > > > > > > > > > > > > > > > on arm-linux-gcc. Is this OK for
> > > > > > > > > > > > > > > > > > > > > > > trunk?
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > While it works for your simple
> > > > > > > > > > > > > > > > > > > > > > cases it is unlikely to work in
> > > > > > > > > > > > > > > > > > > > > > practice since
> > > > > > > > > > > > > > > > > > > > > > your implementation needs the
> > > > > > > > > > > > > > > > > > > > > > assembler options be present at the
> > > > > > > > > > > > > > > > > > > > > > link
> > > > > > > > > > > > > > > > > > > > > > command line. I agree that this
> > > > > > > > > > > > > > > > > > > > > > might be the way for people to go
> > > > > > > > > > > > > > > > > > > > > > when
> > > > > > > > > > > > > > > > > > > > > > they face the issue but then it
> > > > > > > > > > > > > > > > > > > > > > needs to be documented somewhere
> > > > > > > > > > > > > > > > > > > > > > in the manual.
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > That is, with COLLECT_AS_OPTION
> > > > > > > > > > > > > > > > > > > > > > (why singular? I'd expected
> > > > > > > > > > > > > > > > > > > > > > COLLECT_AS_OPTIONS) available to
> > > > > > > > > > > > > > > > > > > > > > cc1 we could stream this string
> > > > > > > > > > > > > > > > > > > > > > to lto_options and re-materialize
> > > > > > > > > > > > > > > > > > > > > > it at link time (and diagnose
> > > > > > > > > > > > > > > > > > > > > > mismatches
> > > > > > > > > > > > > > > > > > > > > > even if we like).
> > > > > > > > > > > > > > > > > > > > > OK. I will try to implement this. So
> > > > > > > > > > > > > > > > > > > > > the idea is if we provide
> > > > > > > > > > > > > > > > > > > > > -Wa,options as part of the lto
> > > > > > > > > > > > > > > > > > > > > compile, this should be available
> > > > > > > > > > > > > > > > > > > > > during link time. Like in:
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > arm-linux-gnueabihf-gcc
> > > > > > > > > > > > > > > > > > > > > -march=armv7-a -mthumb -O2 -flto
> > > > > > > > > > > > > > > > > > > > > -Wa,-mimplicit-it=always,-mthumb -c
> > > > > > > > > > > > > > > > > > > > > test.c
> > > > > > > > > > > > > > > > > > > > > arm-linux-gnueabihf-gcc -flto test.o
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > I am not sure where should we stream
> > > > > > > > > > > > > > > > > > > > > this. Currently, cl_optimization
> > > > > > > > > > > > > > > > > > > > > has all the optimization flag
> > > > > > > > > > > > > > > > > > > > > provided for compiler and it is
> > > > > > > > > > > > > > > > > > > > > autogenerated and all the flags are
> > > > > > > > > > > > > > > > > > > > > integer values. Do you have any
> > > > > > > > > > > > > > > > > > > > > preference or example where this
> > > > > > > > > > > > > > > > > > > > > should be done.
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > In lto_write_options, I'd simply append
> > > > > > > > > > > > > > > > > > > > the contents of COLLECT_AS_OPTIONS
> > > > > > > > > > > > > > > > > > > > (with -Wa, prepended to each of them),
> > > > > > > > > > > > > > > > > > > > then recover them in lto-wrapper
> > > > > > > > > > > > > > > > > > > > for each TU and pass them down to the
> > > > > > > > > > > > > > > > > > > > LTRANS compiles (if they agree
> > > > > > > > > > > > > > > > > > > > for all TUs, otherwise I'd warn and
> > > > > > > > > > > > > > > > > > > > drop them).
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Attached patch streams it and also make
> > > > > > > > > > > > > > > > > > > sure that the options are the
> > > > > > > > > > > > > > > > > > > same for all the TUs. Maybe it is a bit
> > > > > > > > > > > > > > > > > > > restrictive.
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > What is the best place to document
> > > > > > > > > > > > > > > > > > > COLLECT_AS_OPTIONS. We don't seem
> > > > > > > > > > > > > > > > > > > to document COLLECT_GCC_OPTIONS anywhere ?
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Nowhere, it's an implementation detail then.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Attached patch passes regression and also
> > > > > > > > > > > > > > > > > > > fixes the original ARM
> > > > > > > > > > > > > > > > > > > kernel build issue with tumb2.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > 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
> > > > > > > > > > > > > > > > > > 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?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Don't you need to also handle -Xassembler?
> > > > > > > > > > > > > > > > Since -Wa, doesn't work with comma in
> > > > > > > > > > > > > > > > assembler options, like -mfoo=foo1,foo2, one
> > > > > > > > > > > > > > > > needs to use
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > -Xassembler -mfoo=foo1,foo2
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > to pass -mfoo=foo1,foo2 to assembler.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > gcc -flto -O2 -Wa,-mcpu=zzz1 -mcpu=xxx1 -c foo.c
> > > > > > > > > > > > > > > gcc -flto -O2 -Wa,-mcpu=zzz2 -mcpu=xxx2 -c bar.c
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > What should be the option we should provide for
> > > > > > > > > > > > > > > the final
> > > > > > > > > > > > > > > gcc -flto foo.o bar.o -o out
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I think our ultimate aim is to handle this in LTO
> > > > > > > > > > > > > > > partitioning. That
> > > > > > > > > > > > > > > is, we should create partitioning such that each
> > > > > > > > > > > > > > > partition has the
> > > > > > > > > > > > > > > same -Wa options. This could also handle
> > > > > > > > > > > > > > > -Xassembler -mfoo=foo1,foo2
> > > > > > > > > > > > > > > which H.J. Lu wanted. We need to modify the
> > > > > > > > > > > > > > > heuristics and do some
> > > > > > > > > > > > > > > performance testing.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > In the meantime we could push a simpler solution
> > > > > > > > > > > > > > > which is to accept
> > > > > > > > > > > > > > > -Wa option if they are identical. This would fix
> > > > > > > > > > > > > > > at least some of the
> > > > > > > > > > > > > > > reported cases. Trying to work out what is
> > > > > > > > > > > > > > > compatible options, even if
> > > > > > > > > > > > > > > we ask the back-end to do this, is not a
> > > > > > > > > > > > > > > straightforward strategy and
> > > > > > > > > > > > > > > can be a maintenance nightmare. Unless we can
> > > > > > > > > > > > > > > query GNU AS somehow. If
> > > > > > > > > > > > > > > I am missing something please let me know.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > +/* Store switches specified for as with -Wa in
> > > > > > > > > > > > > > COLLECT_AS_OPTIONS
> > > > > > > > > > > > > > + and place that in the environment. */
> > > > > > > > > > > > > > +static void
> > > > > > > > > > > > > > +putenv_COLLECT_AS_OPTIONS (vec<char_p> vec)
> > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > + unsigned ix;
> > > > > > > > > > > > > > + char *opt;
> > > > > > > > > > > > > > + int len = vec.length ();
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > + if (!len)
> > > > > > > > > > > > > > + return;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > + obstack_init (&collect_obstack);
> > > > > > > > > > > > > > + obstack_grow (&collect_obstack,
> > > > > > > > > > > > > > "COLLECT_AS_OPTIONS=",
> > > > > > > > > > > > > > + sizeof ("COLLECT_AS_OPTIONS=") - 1);
> > > > > > > > > > > > > > + obstack_grow (&collect_obstack, "-Wa,", strlen
> > > > > > > > > > > > > > ("-Wa,"));
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > + FOR_EACH_VEC_ELT (vec, ix, opt)
> > > > > > > > > > > > > > + {
> > > > > > > > > > > > > > + obstack_grow (&collect_obstack, opt, strlen
> > > > > > > > > > > > > > (opt));
> > > > > > > > > > > > > > + --len;
> > > > > > > > > > > > > > + if (len)
> > > > > > > > > > > > > > + obstack_grow (&collect_obstack, ",", strlen
> > > > > > > > > > > > > > (","));
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > + xputenv (XOBFINISH (&collect_obstack, char *));
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This missed the null terminator.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Attached patch addresses the review comments I got so
> > > > > > > > > > > > > far.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > + if (len)
> > > > > > > > > > > > + obstack_grow (&collect_obstack, ",", strlen (","));
> > > > > > > > > > > >
> > > > > > > > > > > > Why not sizeof (",") - 1?
> > > > > > > > > > > I guess I copied and pasted it from elsewhere else. We
> > > > > > > > > > > seem to use
> > > > > > > > > > > both. I have changed it now.
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> > > > > > > > > > > > index 9a7bbd0c022..148c52906d1 100644
> > > > > > > > > > > > --- a/gcc/lto-wrapper.c
> > > > > > > > > > > > +++ b/gcc/lto-wrapper.c
> > > > > > > > > > > > @@ -253,6 +253,11 @@ merge_and_complain (struct
> > > > > > > > > > > > cl_decoded_option
> > > > > > > > > > > > **decoded_options,
> > > > > > > > > > > > break;
> > > > > > > > > > > >
> > > > > > > > > > > > default:
> > > > > > > > > > > > + if (foption->opt_index == OPT_Wa_)
> > > > > > > > > > > > + {
> > > > > > > > > > > > + append_option (decoded_options,
> > > > > > > > > > > > decoded_options_count, foption);
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + }
> > > > > > > > > > > > if (!(cl_options[foption->opt_index].flags &
> > > > > > > > > > > > CL_TARGET))
> > > > > > > > > > > > break;
> > > > > > > > > > > >
> > > > > > > > > > > > Why not use "case OPT_Wa_:" here?
> > > > > > > > > > > Done.
> > > > > > > > > > > >
> > > > > > > > > > > > For
> > > > > > > > > > > >
> > > > > > > > > > > > + static const char *collect_as;
> > > > > > > > > > > > + for (unsigned int j = 1; j < count; ++j)
> > > > > > > > > > > > + {
> > > > > > > > > > > > + struct cl_decoded_option *option = &opts[j];
> > > > > > > > > > > > + if (j == 1)
> > > > > > > > > > > > + collect_as = NULL;
> > > > > > > > > > > >
> > > > > > > > > > > > why not simply
> > > > > > > > > > > >
> > > > > > > > > > > > const char *collect_as = NULL?
> > > > > > > > > > >
> > > > > > > > > > > I wanted to make sure that if we call this from multiple
> > > > > > > > > > > places, it
> > > > > > > > > > > still works. I guess it is still going to be the same. I
> > > > > > > > > > > have changed
> > > > > > > > > > > it now as you have suggested.
> > > > > > > > > > >
> > > > > > > > > > > Is this revised patch OK? I will do a fresh bootstrap and
> > > > > > > > > > > regression
> > > > > > > > > > > testing before committing.
> > > > > > > > > >
> > > > > > > > > > In putenv_COLLECT_AS_OPTIONS you'll happily make
> > > > > > > > > > -Wa,-march=foo,bar out of -Xassembler -march=foo,bar which
> > > > > > > > > > will later cause us to fail to assemble with unknown
> > > > > > > > > > assembler options.
> > > > > > > > > > May I suggest to instead always use -Xassembler syntax in
> > > > > > > > > > COLLECT_AS_OPTIONS? Please also make sure to quote
> > > > > > > > > > options the same way set_collect_gcc_options does
> > > > > > > > > > (with '', separated by spaces). Then the lto-opts.c part
> > > > > > > > > > becomes "easier" as you can simply copy the string to the
> > > > > > > > > > obstack without wrapping it again with
> > > > > > > > > > append_to_collect_gcc_options.
> > > > > > > > > >
> > > > > > > > > > In lto-wrapper you then only have to handle OPT_Xassembler.
> > > > > > > > > >
> > > > > > > > > > You simply end up appending _all_ assembler options in order
> > > > > > > > > > of TUs processed by lto-wrapper to the final command (N
> > > > > > > > > > times
> > > > > > > > > > even if exactly the same). I'm also not sure how you can
> > > > > > > > > > check
> > > > > > > > > > for positional equivalence (or if we even should). With -Wa
> > > > > > > > > > we could compare the full option string but with separate
> > > > > > > > > > -Xassembler
> > > > > > > > > > we're having a more difficult task here. OTOH your patch
> > > > > > > > > > doesn't
> > > > > > > > > > do any comparing here.
> > > > > > > > > >
> > > > > > > > > > Your append_compiler_wa_options should be merged into
> > > > > > > > > > append_compiler_options, passing -Xassembler through.
> > > > > > > > > Hi Richard,
> > > > > > > > > Since Kugan has left Linaro (and GCC), I'd like to take up
> > > > > > > > > this task.
> > > > > > > > > I have modified his patch to always pass assembler options
> > > > > > > > > via -Xassembler.
> > > > > > > > > Does it look OK ?
> > > > > > > > >
> > > > > > > > > I am not sure how we should proceed with error-checking for
> > > > > > > > > Xassembler ?
> > > > > > > > > In lto-wrapper, I suppose, we can append all Xassembler
> > > > > > > > > options for a
> > > > > > > > > TU into a single string, and then
> > > > > > > > > do strcmp similar to previous patch(es) doing strcmp for -Wa
> > > > > > > > > options
> > > > > > > > > string, although not sure if that's a good idea.
> > > > > > > >
> > > > > > > > I think there are multiple issues with the main one being how to
> > > > > > > > actually interpret -Xassembler in the LTO context.
> > > > > > > >
> > > > > > > > First let me point out some bits in the COLLECT_AS_OPTIONS
> > > > > > > > parts.
> > > > > > > >
> > > > > > > > + FOR_EACH_VEC_ELT (vec, ix, opt)
> > > > > > > > + {
> > > > > > > > + obstack_grow (&collect_obstack, "\'-Xassembler\' ",
> > > > > > > > + strlen ("\'-Xassembler\' "));
> > > > > > > >
> > > > > > > > quoting of -Xassembler is not necessary.
> > > > > > > >
> > > > > > > > + obstack_1grow (&collect_obstack, '\'');
> > > > > > > > + obstack_grow (&collect_obstack, opt, strlen (opt));
> > > > > > > > + obstack_grow (&collect_obstack, "\' ", 2);
> > > > > > > >
> > > > > > > > This adds a stray space after the last option.
> > > > > > > >
> > > > > > > > Note that COLLECT_AS_OPTIONS gives the impression of listing
> > > > > > > > assembler options but the above adds GCC driver options -
> > > > > > > > assembler
> > > > > > > > options prepended by -Xassembler. IMHO we should drop the
> > > > > > > > -Xassembler emission from the above loop and simply emit the
> > > > > > > > plain
> > > > > > > > assembler options. That requires adjustments to
> > > > > > > > lto_write_options,
> > > > > > > > adding those -Xassembler options.
> > > > > > > >
> > > > > > > > + char *asm_opts = XOBFINISH (&collect_obstack, char *);
> > > > > > > > + xputenv (XOBFINISH (&collect_obstack, char *));
> > > > > > > > + xputenv (asm_opts);
> > > > > > > >
> > > > > > > > That outputs the ENV twice.
> > > > > > > >
> > > > > > > > Note that we record things like --version or --help into
> > > > > > > > assembler_options but I'm not sure the merging of assembler
> > > > > > > > options should be affected on whether one TU was compiled with
> > > > > > > > -v
> > > > > > > > or not. This might mean simply pruning those in lto-options.c
> > > > > > > > (not listing them in COLLECT_AS_OPTIONS wouldn't tell the
> > > > > > > > truth).
> > > > > > > >
> > > > > > > > @@ -252,6 +252,10 @@ merge_and_complain (struct
> > > > > > > > cl_decoded_option
> > > > > > > > **decoded_options,
> > > > > > > > case OPT_SPECIAL_input_file:
> > > > > > > > break;
> > > > > > > >
> > > > > > > > + case OPT_Xassembler:
> > > > > > > > + append_option (decoded_options,
> > > > > > > > decoded_options_count, foption);
> > > > > > > > + break;
> > > > > > > > +
> > > > > > > >
> > > > > > > > this adds the same option over-and-over again, possibly
> > > > > > > > becoming unwieldly.
> > > > > > > > Most of the function also assumes that option position isn't
> > > > > > > > important
> > > > > > > > which might or might not be true. So I think a better course
> > > > > > > > of action
> > > > > > > > would be to not handle Xassembler in the above loop but do a
> > > > > > > > separate
> > > > > > > > one checking 1:1 equality of passed assembler options like
> > > > > > > >
> > > > > > > > /* Verify -Xassembler options are the same on all TUs. */
> > > > > > > > j = 0;
> > > > > > > > i = 0;
> > > > > > > > unsigned Xascount = 0;
> > > > > > > > while (j < *decoded_options_count && i <
> > > > > > > > fdeconded_options_count)
> > > > > > > > {
> > > > > > > > while (fdecoded_options[i].opt_index != OPT_Xassembler)
> > > > > > > > ++i;
> > > > > > > > same for *decoded_options
> > > > > > > > if (stray Xassembler on one side)
> > > > > > > > fatal_error (...);
> > > > > > > > if (strcmp (...) != 0)
> > > > > > > > fatal_error (...);
> > > > > > > > }
> > > > > > > >
> > > > > > > > which means we use the -Xassembler options from the first TU and
> > > > > > > > above only verify those match those from all other TUs.
> > > > > > > Hi Richard,
> > > > > > > Thanks for the suggestions, I tried to address them in the
> > > > > > > attached patch.
> > > > > > > It now gives errors on following cases during link command:
> > > > > > >
> > > > > > > 1]
> > > > > > > gcc -O -flto -c -Xassembler -mfoo f1.c
> > > > > > > gcc -O -flto -c f2.c
> > > > > > > gcc -O -flto f1.o f2.o
> > > > > > >
> > > > > > > 2]
> > > > > > > gcc -O -flto -c -Xassembler -mfoo f1.c
> > > > > > > gcc -O -flto -c -Xassembler -mbar f2.c
> > > > > > > gcc -O -flto f1.o f2.o
> > > > > > >
> > > > > > > 3]
> > > > > > > gcc -O -flto -c -Xassembler -mfoo -Xassembler -mbar f1.c
> > > > > > > gcc -O -flto -c -Xassembler -mbar -Xassembler -mfoo f2.c
> > > > > > > gcc -O -flto f1.o f2.o
> > > > > > >
> > > > > > > 4]
> > > > > > > gcc -O -flto -c f1.c
> > > > > > > gcc -O -flto -Xassembler -mfoo f1.c
> > > > > > > gcc -O -flto f1.o f2.o
> > > > > > >
> > > > > > > 5]
> > > > > > > gcc -O -flto -Xassembler -mfoo f1.c
> > > > > > > gcc -O -flto -Xassembler -mfoo f2.c
> > > > > > > gcc -O -flto -Xassembler -mbar f1.o f2.o
> > > > > > >
> > > > > > > The following correct case works:
> > > > > > > gcc -O -flto -Xassembler -mfoo f1.c
> > > > > > > gcc -O -flto -Xassembler -mfoo f2.c
> > > > > > > gcc -O -flto f1.o f2.o
> > > > > >
> > > > > > I think 5] should work as well and behave as -mfoo -mbar at
> > > > > > assembler time.
> > > > > > Add
> > > > > >
> > > > > > 6]
> > > > > > gcc -O -flto f1.c
> > > > > > gcc -O -flto f2.c
> > > > > > gcc -O -flto -Xassembler -mfoo f1.o f2.o
> > > > > >
> > > > > > which should work as well (I think even this use doesn't work right
> > > > > > now?)
> > > > > >
> > > > > > > Could you please suggest how to add the above cases in dejaGNU
> > > > > > > format ?
> > > > > > > I am not sure how to write multiple files test with dejaGNU.
> > > > > >
> > > > > > look at multi-file testcases in gcc.dg/lto/, use testcase_0.c
> > > > > > testcase_1.c,
> > > > > > you can use dg-additional-options to pass -Xassembler (but
> > > > > > eventually
> > > > > > that doesn't work on the first TU), also there's some additional
> > > > > > option
> > > > > > for the link step directive (just look into the existing tests).
> > > > > >
> > > > > > > Also, do you think it's better if we append xassembler options to
> > > > > > > COLLECT_GCC itself rather
> > > > > > > than maintaining COLLECT_AS_OPTIONS with "Xassembler" prepended ?
> > > > > > > Because in both lto_write_options,
> > > > > > > and run_gcc, I am reconstructing "-Xassembler" <opt> for each opt
> > > > > > > in
> > > > > > > COLLECT_AS_OPTIONS.
> > > > > > >
> > > > > > > I am not quite sure how Xassembler options were added to
> > > > > > > fdecoded_options because I am not appending them
> > > > > > > explicitly. IIUC, find_and_merge_options will add -Xassembler to
> > > > > > > fdecoded_options when it's NULL ?
> > > > > > > if (!fdecoded_options)
> > > > > > > {
> > > > > > > fdecoded_options = f2decoded_options;
> > > > > > > fdecoded_options_count = f2decoded_options_count;
> > > > > > > }
> > > > > > > since merge_and_complain does not handle OPT_Xassembler.
> > > > > >
> > > > > > Yes.
> > > > > >
> > > > > > Comments on the patch. First avoid <algorithm>, just use MIN/MAX
> > > > > > if really needed. I'd elide xassembler_opts[_count]. For 6] you
> > > > > > want
> > > > > > to unconditionally append the options.
> > > > > >
> > > > > > In find_and_merge_options I'd have avoided xassembler_opts[_count]
> > > > > > by simply adding another nested loop over both decoded options
> > > > > > requiring matching up OPT_Xassembler 1:1.
> > > > > Hi Richard,
> > > > > Thanks for the suggestions. The current patch removes
> > > > > xasembler_opts[_count] and uses nested loop
> > > > > for comparison.
> > > > > In find_and_merge_options, I used curr_xopts[_count] to hold all the
> > > > > options passed to current TU.
> > > > > IIUC, f2decoded_options will be overwritten each time in the loop
> > > > > while fetching strings from .opts section, and will
> > > > > not contain all options passed to current TU. And I dropped validating
> > > > > for cmdline opts which passes 5] and 6].
> > > > > Does that look OK ?
> > > >
> > > > Huh, it looks odd. Why didn't you simply add the loop to
> > > > merge_and_complain?
> > > > That way you verify each TUs arguments against the first TUs.
> > > My concern was that it might perhaps not work if .opts section in LTO
> > > object file
> > > contained multiple strings ? So fdecoded_options in merge_and_complain
> > > may not hold all options passed to TU. Currently that isn't an issue,
> > > since the section
> > > contains only one string (since we append '\0' once at end of
> > > lto_write_options).
> > > I was wondering will this break if that changed and .opts contains
> > > multiple strings instead ?
> > > In attached patch, I placed the loop in merge_and_complain.
> > > Does that look OK ?
> >
> > + for (unsigned i = 0; i < f2decoded_options_count; i++)
> > + append_option (&curr_xopts, &curr_xopts_count,
> > &f2decoded_options[i]);
> > +
> >
> > why this?
> >
> > + while (j < curr_xopts_count
> > + && curr_xopts[j].opt_index != OPT_Xassembler)
> > + j++;
> >
> > just walk over f2decoded_options here?
> Hi Richard,
> Um, did you perhaps look at the previous version ?
> In the last (and current) patch, I had removed the loop from
> find_and_merge_options
> and placed it in in merge_and_complain instead which avoids curr_xopts[_count]
Huh, maybe. It looks OK now (see comments below about -m handling).
> >
> > obstack_grow (&temporary_obstack, " '-Xassembler' ",
> > + strlen (" '-Xassembler' "));
> >
> > there's no need to quote '-Xassembler'.
> >
> > + tok = strtok_r (NULL, " ", &saveptr);
> >
> > hmm, so we quote -Xassembler arguments with ' but you split apart on " ",
> > that's not going to work. See get_options_from_collect_gcc_options on
> > how to parse it. I suggest to refactor that function to be usable in this
> > context. The same issue is present in lto_write_options, so the function
> > should eventually reside in opts-common.c?
> >
> Thanks for the suggestions. I refactored parsing code from
> get_options_from_collect_gcc_options
> and put it into parse_options_from_collect_gcc_options, which is then
> used by both lto_write_options
> and get_options_from_collect_gcc_options.
> Does it look OK ?
+ parse_options_from_collect_gcc_options (collect_gcc_options,
+ &argv_obstack, &argc);
you wanted to pass 'false' here for the defaulted arg? I think it would be much
cleaner to elide the init_obstack parameter and initialize the obstack in the
callers where required.
> After dropping quote from -Xassembler, it seems not to be inserted in
> argv in get_options_from_collect_gcc_options.
> So I kept it as-is.
Fair enough.
> > > >
> > > > >
> > > > > How should we handle conflicting argument to options passed on
> > > > > cmdline ?
> > > > > For eg:
> > > > > gcc -O -flto -Xassembler -mfoo=arg1 f1.c -o f1.o
> > > > > gcc -O -flto -Xassembler -mfoo=arg1 f2.c -o f2.o
> > > > > gcc -O -flto -Xassembler -mfoo=arg2 f1.o f2.o
> > > > > Should we complain that arg1, arg2 differ or let arg2 take precedence
> > > > > over arg1 for -mfoo ?
> > > > > (It seems currently, the patch does latter).
> > > >
> > > > I think appending the linker -Xassembler makes most sense, appropriately
> > > > diagnosing is difficult here and repeating compile-time assembler
> > > > options
> > > > will be common.
> > > OK, thanks for the clarification.
> >
> > So I am worried that we make programs fail to compile with -flto with this
> > patch due to the fatal_error on mismatched assembler options. Consider
> > targets that, via specs processing, append assembler options from
> > -m options?
> Hmm, would ignoring Xassembler options that don't begin with "-m" be
> an acceptable solution ?
> In the patch, I am skipping Xassembler args that don't begin with "-m".
Can you check whether specs processing introudced assembler options appear
in COLLECT_AS_OPTIONS? For x86 that's for example -msse2avx which
is translated to -msse2avx. For arm it's stuff like -mbig-endian which gets
translated to -EB (no -m ...). For those the peferece is probably to keep
the GCC driver option rather than turning them into -Xassembler ones.
I don't think we want to skip assembler options not matching -m (see -EB ...).
We may want to skip obviously harmless ones though, but not sure how
to identify them :/ -Xassembler -v, --version or -version might be obvious
candidates but of course the actual harmless options can not only differ
from target to target but also from assembler to assembler...
That said, I guess it's fine if only explicitely given options end up in
COLLECT_AS_OPTIONS, not ones via specs processing so that's something
to verify.
Richard.
> Thanks,
> Prathamesh
> >
> > Richard.
> >
> > > Thanks,
> > > Prathamesh
> > > >
> > > > Richard.
> > > >
> > > > >
> > > > > I am still looking into the tests part, will address that in next
> > > > > patch.
> > > > >
> > > > > Thanks,
> > > > > Prathamesh
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Richard.
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Prathamesh
> > > > > > > >
> > > > > > > > Richard.
> > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Prathamesh
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Richard.
> > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > > > Kugan
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > H.J.
