On Mon, 17 Feb 2020 at 19:52, Richard Biener <richard.guent...@gmail.com> wrote:
>
> On Mon, Feb 17, 2020 at 10:28 AM Prathamesh Kulkarni
> <prathamesh.kulka...@linaro.org> wrote:
> >
> > On Thu, 6 Feb 2020 at 20:03, Prathamesh Kulkarni
> > <prathamesh.kulka...@linaro.org> wrote:
> > >
> > > On Thu, 6 Feb 2020 at 18:42, Richard Biener <richard.guent...@gmail.com> 
> > > wrote:
> > > >
> > > > On Thu, Feb 6, 2020 at 1:48 PM Prathamesh Kulkarni
> > > > <prathamesh.kulka...@linaro.org> wrote:
> > > > >
> > > > > On Tue, 4 Feb 2020 at 19:44, Richard Biener 
> > > > > <richard.guent...@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Feb 3, 2020 at 12:37 PM Prathamesh Kulkarni
> > > > > > <prathamesh.kulka...@linaro.org> wrote:
> > > > > > >
> > > > > > > On Thu, 30 Jan 2020 at 19:10, Richard Biener 
> > > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jan 30, 2020 at 5:31 AM Prathamesh Kulkarni
> > > > > > > > <prathamesh.kulka...@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, 28 Jan 2020 at 17:17, Richard Biener 
> > > > > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, Jan 24, 2020 at 7:04 AM Prathamesh Kulkarni
> > > > > > > > > > <prathamesh.kulka...@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Mon, 20 Jan 2020 at 15:44, Richard Biener 
> > > > > > > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Jan 8, 2020 at 11:20 AM Prathamesh Kulkarni
> > > > > > > > > > > > <prathamesh.kulka...@linaro.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, 5 Nov 2019 at 17:38, Richard Biener 
> > > > > > > > > > > > > <richard.guent...@gmail.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Tue, Nov 5, 2019 at 12:17 AM Kugan 
> > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Tue, 5 Nov 2019 at 03:57, H.J. Lu 
> > > > > > > > > > > > > > > <hjl.to...@gmail.com> wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Sun, Nov 3, 2019 at 6:45 PM Kugan 
> > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Thanks for the reviews.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > On Sat, 2 Nov 2019 at 02:49, H.J. Lu 
> > > > > > > > > > > > > > > > > <hjl.to...@gmail.com> wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Thu, Oct 31, 2019 at 6:33 PM Kugan 
> > > > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> wrote:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > On Wed, 30 Oct 2019 at 03:11, H.J. Lu 
> > > > > > > > > > > > > > > > > > > <hjl.to...@gmail.com> wrote:
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > On Sun, Oct 27, 2019 at 6:33 PM Kugan 
> > > > > > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> 
> > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > On Wed, 23 Oct 2019 at 23:07, Richard 
> > > > > > > > > > > > > > > > > > > > > Biener <richard.guent...@gmail.com> 
> > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > On Mon, Oct 21, 2019 at 10:04 AM 
> > > > > > > > > > > > > > > > > > > > > > Kugan Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org> 
> > > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > Thanks for the pointers.
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > On Fri, 11 Oct 2019 at 22:33, 
> > > > > > > > > > > > > > > > > > > > > > > Richard Biener 
> > > > > > > > > > > > > > > > > > > > > > > <richard.guent...@gmail.com> 
> > > > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 11, 2019 at 6:15 AM 
> > > > > > > > > > > > > > > > > > > > > > > > Kugan Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org>
> > > > > > > > > > > > > > > > > > > > > > > >  wrote:
> > > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > > Hi Richard,
> > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the review.
> > > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 2 Oct 2019 at 20:41, 
> > > > > > > > > > > > > > > > > > > > > > > > > Richard Biener 
> > > > > > > > > > > > > > > > > > > > > > > > > <richard.guent...@gmail.com> 
> > > > > > > > > > > > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Oct 2, 2019 at 
> > > > > > > > > > > > > > > > > > > > > > > > > > 10:39 AM Kugan 
> > > > > > > > > > > > > > > > > > > > > > > > > > Vivekanandarajah
> > > > > > > > > > > > > > > > > > > > > > > > > > <kugan.vivekanandara...@linaro.org>
> > > > > > > > > > > > > > > > > > > > > > > > > >  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.
> > > Oops, sorry, yes false was correct there. I removed defaulted arg, and
> > > initialize obstack
> > > by called in attached patch.
> > > >
> > > > > 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.
> > > Ah, didn't know about this. IIUC, you mean the options in
> > > ${builddir}/gcc/specs ?
> > > -mbig-endian translates to -EB and -mlittle-endian to -EL etc.
> > > I passed -O -flto -mbig-endian and the driver did not seem to
> > > translate it into Xassembler opt
> > > (collect_as_options was NULL), but passed -EB to assembler.
> > > >
> > > > 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.
> > > Indeed, that seems to be the case to me.
> > > Does the attached patch look OK ?
> > Hi Richard, ping ?
> > Just wondering if this patch'd be suitable for stage-4 ?
> > Altho not exactly a regression, it blocks building kernel with LTO for
> > ARM targets,
>
> I'm curious which assembler option is needed for kernel build and if
> just handling link-time -Wa,... would be enough here (thus the
> COLLECT_AS_OPTIONS export plus the run_gcc lto-wrapper hunk).
The option was -mimplicit-it=always:
https://www.spinics.net/lists/arm-kernel/msg635611.html
PR78353 contains reduced test-case of same issue.
IIRC, Kugan's original patch, which only handled -Wa at link-time,
fixed it.
>
> > and I was hoping if we could get this pushed in GCC-10.
>
> So we discussed this locally a bit and agreed that issueing a fatal_error
> on option mismatch isn't good progression.  Instead can you output
> a non-fatal diagnostic to stderr and drop all -Xassembler options when
> we see a  mismatch (to get back previous behavior)?  To be visible
> the user unfortunately will have to pass -Wl,-debug to the link
> command-line but that's better than nothing.
>
> So all but the merge_and_complain hunk are OK I think and that hunk
> needs some adjustment to avoid failing the link.
>
> I think we also should adjust documentation like with
>
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 3e47d06f0d5..6f0698b16bf 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -11159,6 +11159,12 @@ conflicting translation units.  Specifically
>  precedence; and for example @option{-ffp-contract=off} takes precedence
>  over @option{-ffp-contract=fast}.  You can override them at link time.
>
> +When you need to pass options to the assembler via @option{-Wa} or
> +@option{-Xassembler} make sure to either compile such translation
> +units with @option{-fno-lto} or consistently use the same assembler
> +options on all translation units.  You can alternatively also
> +specify assembler options at LTO link time.
> +
>  To enable debug info generation you need to supply @option{-g} at
>  compile time.  If any of the input files at link time were built
>  with debug info generation enabled the link will enable debug info
Thanks for the suggestions, I have updated the patch accordingly.
Does it look OK ?
Thanks,
Prathamesh
>
> Thanks,
> Richard.
>
> > Thanks,
> > Prathamesh
> > >
> > > Thanks,
> > > Prathamesh
> > > >
> > > > 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.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index bbd03f87c67..c8545d31519 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -11101,6 +11101,12 @@ conflicting translation units.  Specifically
 precedence; and for example @option{-ffp-contract=off} takes precedence
 over @option{-ffp-contract=fast}.  You can override them at link time.
 
+When you need to pass options to the assembler via @option{-Wa} or
+@option{-Xassembler} make sure to either compile such translation
+units with @option{-fno-lto} or consistently use the same assembler
+options on all translation units.  You can alternatively also
+specify assembler options at LTO link time.
+
 To enable debug info generation you need to supply @option{-g} at
 compile time.  If any of the input files at link time were built
 with debug info generation enabled the link will enable debug info
diff --git a/gcc/gcc.c b/gcc/gcc.c
index effc384f3ef..9f790db0daf 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -5242,6 +5242,34 @@ do_specs_vec (vec<char_p> vec)
     }
 }
 
+/* Add options passed via -Xassembler or -Wa to COLLECT_AS_OPTIONS.  */
+
+static void
+putenv_COLLECT_AS_OPTIONS (vec<char_p> vec)
+{
+  if (vec.is_empty ())
+     return;
+
+  obstack_init (&collect_obstack);
+  obstack_grow (&collect_obstack, "COLLECT_AS_OPTIONS=",
+		strlen ("COLLECT_AS_OPTIONS="));
+
+  char *opt;
+  unsigned ix;
+
+  FOR_EACH_VEC_ELT (vec, ix, opt)
+    {
+      obstack_1grow (&collect_obstack, '\'');
+      obstack_grow (&collect_obstack, opt, strlen (opt));
+      obstack_1grow (&collect_obstack, '\'');
+      if (ix < vec.length () - 1)
+	obstack_1grow(&collect_obstack, ' ');
+    }
+
+  obstack_1grow (&collect_obstack, '\0');
+  xputenv (XOBFINISH (&collect_obstack, char *));
+}
+
 /* Process the sub-spec SPEC as a portion of a larger spec.
    This is like processing a whole spec except that we do
    not initialize at the beginning and we do not supply a
@@ -7363,6 +7391,7 @@ driver::main (int argc, char **argv)
   global_initializations ();
   build_multilib_strings ();
   set_up_specs ();
+  putenv_COLLECT_AS_OPTIONS (assembler_options);
   putenv_COLLECT_GCC (argv[0]);
   maybe_putenv_COLLECT_LTO_WRAPPER ();
   maybe_putenv_OFFLOAD_TARGETS ();
diff --git a/gcc/lto-opts.c b/gcc/lto-opts.c
index 90bfde5a8fd..87e916a2741 100644
--- a/gcc/lto-opts.c
+++ b/gcc/lto-opts.c
@@ -163,6 +163,12 @@ lto_write_options (void)
 	append_to_collect_gcc_options (&temporary_obstack, &first_p,
 				       option->canonical_option[j]);
     }
+
+  const char *collect_as_options = getenv ("COLLECT_AS_OPTIONS");
+  if (collect_as_options)
+    prepend_xassembler_to_collect_as_options (collect_as_options,
+					      &temporary_obstack);
+
   obstack_grow (&temporary_obstack, "\0", 1);
   args = XOBFINISH (&temporary_obstack, char *);
   lto_write_data (args, strlen (args) + 1);
diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index fe8f292f877..a01bfd244d5 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -73,6 +73,7 @@ static char *offload_objects_file_name;
 static char *makefile;
 static unsigned int num_deb_objs;
 static const char **early_debug_object_names;
+static bool xassembler_options_error = false;
 
 const char tool_name[] = "lto-wrapper";
 
@@ -137,42 +138,14 @@ get_options_from_collect_gcc_options (const char *collect_gcc,
 				      unsigned int *decoded_options_count)
 {
   struct obstack argv_obstack;
-  char *argv_storage;
   const char **argv;
-  int j, k, argc;
+  int argc;
 
-  argv_storage = xstrdup (collect_gcc_options);
   obstack_init (&argv_obstack);
   obstack_ptr_grow (&argv_obstack, collect_gcc);
 
-  for (j = 0, k = 0; argv_storage[j] != '\0'; ++j)
-    {
-      if (argv_storage[j] == '\'')
-	{
-	  obstack_ptr_grow (&argv_obstack, &argv_storage[k]);
-	  ++j;
-	  do
-	    {
-	      if (argv_storage[j] == '\0')
-		fatal_error (input_location,
-			     "malformed %<COLLECT_GCC_OPTIONS%>");
-	      else if (strncmp (&argv_storage[j], "'\\''", 4) == 0)
-		{
-		  argv_storage[k++] = '\'';
-		  j += 4;
-		}
-	      else if (argv_storage[j] == '\'')
-		break;
-	      else
-		argv_storage[k++] = argv_storage[j++];
-	    }
-	  while (1);
-	  argv_storage[k++] = '\0';
-	}
-    }
-
-  obstack_ptr_grow (&argv_obstack, NULL);
-  argc = obstack_object_size (&argv_obstack) / sizeof (void *) - 1;
+  parse_options_from_collect_gcc_options (collect_gcc_options,
+					  &argv_obstack, &argc);
   argv = XOBFINISH (&argv_obstack, const char **);
 
   decode_cmdline_options_to_array (argc, (const char **)argv, CL_DRIVER,
@@ -512,6 +485,36 @@ merge_and_complain (struct cl_decoded_option **decoded_options,
       }
    else
      j++;
+
+  if (!xassembler_options_error)
+    for (i = j = 0; ; i++, j++)
+      {
+	for (; i < *decoded_options_count; i++)
+	  if ((*decoded_options)[i].opt_index == OPT_Xassembler)
+	    break;
+
+	for (; j < fdecoded_options_count; j++)
+	  if (fdecoded_options[j].opt_index == OPT_Xassembler)
+	    break;
+
+	if (i == *decoded_options_count && j == fdecoded_options_count)
+	  break;
+	else if (i < *decoded_options_count && j == fdecoded_options_count)
+	  fatal_error (input_location, "Extra option to -Xassembler: %s.",
+		       (*decoded_options)[i].arg);
+	else if (i == *decoded_options_count && j < fdecoded_options_count)
+	  fatal_error (input_location, "Extra option to -Xassembler: %s.",
+		       fdecoded_options[j].arg);
+	else if (strcmp ((*decoded_options)[i].arg, fdecoded_options[j].arg))
+	  {
+	    warning_at (input_location, 0,
+			"Options to Xassembler do not match: %s, %s,"
+			" dropping all -Xassembler and -Wa options.",
+			(*decoded_options)[i].arg, fdecoded_options[j].arg);
+	    xassembler_options_error = true;
+	    break;
+	  }
+      }
 }
 
 /* Auxiliary function that frees elements of PTR and PTR itself.
@@ -626,6 +629,11 @@ append_compiler_options (obstack *argv_obstack, struct cl_decoded_option *opts,
 	case OPT_Os:
 	  break;
 
+	case OPT_Xassembler:
+	  if (xassembler_options_error)
+	    continue;
+	  break;
+
 	default:
 	  if (!(cl_options[option->opt_index].flags & CL_TARGET))
 	    continue;
@@ -1251,7 +1259,8 @@ run_gcc (unsigned argc, char *argv[])
   const char **argv_ptr;
   char *list_option_full = NULL;
   const char *linker_output = NULL;
-  const char *collect_gcc, *collect_gcc_options;
+  const char *collect_gcc;
+  char *collect_gcc_options;
   int parallel = 0;
   int jobserver = 0;
   int auto_parallel = 0;
@@ -1281,6 +1290,25 @@ run_gcc (unsigned argc, char *argv[])
   if (!collect_gcc_options)
     fatal_error (input_location,
 		 "environment variable %<COLLECT_GCC_OPTIONS%> must be set");
+
+  char *collect_as_options = getenv ("COLLECT_AS_OPTIONS");
+
+  /* Prepend -Xassembler to each option, and append the string
+     to collect_gcc_options.  */
+  if (collect_as_options)
+    {
+      obstack temporary_obstack;
+      obstack_init (&temporary_obstack);
+
+      prepend_xassembler_to_collect_as_options (collect_as_options,
+						&temporary_obstack);
+      obstack_1grow (&temporary_obstack, '\0');
+
+      char *xassembler_opts_string
+	= XOBFINISH (&temporary_obstack, char *);
+      strcat (collect_gcc_options, xassembler_opts_string);
+    }
+
   get_options_from_collect_gcc_options (collect_gcc, collect_gcc_options,
 					&decoded_options,
 					&decoded_options_count);
diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index 112de159cce..de9510abd64 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -1739,3 +1739,69 @@ control_warning_option (unsigned int opt_index, int kind, const char *arg,
 	}
     }
 }
+
+/* Parse options in COLLECT_GCC_OPTIONS and push them on ARGV_OBSTACK.
+   Store number of arguments into ARGC_P.  */
+
+void
+parse_options_from_collect_gcc_options (const char *collect_gcc_options,
+					obstack *argv_obstack,
+					int *argc_p)
+{
+  char *argv_storage = xstrdup (collect_gcc_options);
+  int j, k;
+
+  for (j = 0, k = 0; argv_storage[j] != '\0'; ++j)
+    {
+      if (argv_storage[j] == '\'')
+	{
+	  obstack_ptr_grow (argv_obstack, &argv_storage[k]);
+	  ++j;
+	  do
+	    {
+	      if (argv_storage[j] == '\0')
+		fatal_error (input_location,
+			     "malformed %<COLLECT_GCC_OPTIONS%>");
+	      else if (strncmp (&argv_storage[j], "'\\''", 4) == 0)
+		{
+		  argv_storage[k++] = '\'';
+		  j += 4;
+		}
+	      else if (argv_storage[j] == '\'')
+		break;
+	      else
+		argv_storage[k++] = argv_storage[j++];
+	    }
+	  while (1);
+	  argv_storage[k++] = '\0';
+	}
+    }
+
+  obstack_ptr_grow (argv_obstack, NULL);
+  *argc_p = obstack_object_size (argv_obstack) / sizeof (void *) - 1;
+}
+
+/* Prepend -Xassembler for each option in COLLECT_AS_OPTIONS,
+   and push on O.  */
+
+void prepend_xassembler_to_collect_as_options (const char *collect_as_options,
+					       obstack *o)
+{
+  obstack opts_obstack;
+  int opts_count;
+
+  obstack_init (&opts_obstack);
+  parse_options_from_collect_gcc_options (collect_as_options,
+					  &opts_obstack, &opts_count);
+  const char **assembler_opts = XOBFINISH (&opts_obstack, const char **);
+
+  for (int i = 0; i < opts_count; i++)
+    {
+      obstack_grow (o, " '-Xassembler' ",
+		    strlen (" '-Xassembler' "));
+      const char *opt = assembler_opts[i];
+      obstack_1grow (o, '\'');
+      obstack_grow (o, opt, strlen (opt));
+      obstack_1grow (o, '\'');
+    }
+}
diff --git a/gcc/opts.h b/gcc/opts.h
index c6ad6c70464..8f594b46e33 100644
--- a/gcc/opts.h
+++ b/gcc/opts.h
@@ -460,6 +460,11 @@ extern bool parse_and_check_align_values (const char *flag,
 					  bool report_error,
 					  location_t loc);
 
+extern void parse_options_from_collect_gcc_options (const char *, obstack *,
+						    int *);
+
+extern void prepend_xassembler_to_collect_as_options (const char *, obstack *);
+
 /* Set OPTION in OPTS to VALUE if the option is not set in OPTS_SET.  */
 
 #define SET_OPTION_IF_UNSET(OPTS, OPTS_SET, OPTION, VALUE) \
diff --git a/gcc/testsuite/gcc.target/arm/pr78353-1.c b/gcc/testsuite/gcc.target/arm/pr78353-1.c
new file mode 100644
index 00000000000..aec0fb0cbfd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr78353-1.c
@@ -0,0 +1,8 @@
+/* { dg-do link }  */
+/* { dg-options "-march=armv7-a -mthumb -O2 -flto -Wa,-mimplicit-it=always" }  */
+
+int main(int x)
+{
+  asm("teq %0, #0; addne %0, %0, #1" : "=r" (x));
+  return x;
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr78353-2.c b/gcc/testsuite/gcc.target/arm/pr78353-2.c
new file mode 100644
index 00000000000..18a90e8834e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr78353-2.c
@@ -0,0 +1,9 @@
+/* { dg-do link }  */
+/* { dg-options "-march=armv7-a -mthumb -O2 -flto -Wa,-mimplicit-it=always,-mthumb" }  */
+
+int main(int x)
+{
+  asm("teq %0, #0; addne %0, %0, #1" : "=r" (x));
+  return x;
+}
+

Reply via email to