On Tue, 18 Feb 2020 at 19:40, Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Feb 18, 2020 at 1:52 PM Prathamesh Kulkarni > <prathamesh.kulka...@linaro.org> wrote: > > > > 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 ? > > + 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)) > > please use warning () here, too (and set xassembler_options_error). > > + 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); > > input_location is not meaningful here so just omit it by using warning (). > > + case OPT_Xassembler: > > Add > > /* When we detected a mismatch in assembler options between > the input CUs > fall back to previous behavior of ignoring them. */ > + if (xassembler_options_error) > + continue; > + break; > > OK with those changes. Did you try if the diagnostics are visible > (when you add -Wl,-debug or/and -Wl,-v to the link command?) Made the changes in attached patch, thanks. The diagnostics are visible with -Wl,-v and -Wl,-debug. LTO bootstrap in progress on x86_64 and arm. OK to commit if passes ?
Thanks, Prathamesh > Richard. > > > 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..6e3f294257e 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,45 @@ 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) + { + warning (0, "Extra option to -Xassembler: %s," + " dropping all -Xassembler and -Wa options.", + (*decoded_options)[i].arg); + xassembler_options_error = true; + break; + } + else if (i == *decoded_options_count && j < fdecoded_options_count) + { + warning (0, "Extra option to -Xassembler: %s," + " dropping all -Xassembler and -Wa options.", + fdecoded_options[j].arg); + xassembler_options_error = true; + break; + } + else if (strcmp ((*decoded_options)[i].arg, fdecoded_options[j].arg)) + { + warning (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 +638,13 @@ append_compiler_options (obstack *argv_obstack, struct cl_decoded_option *opts, case OPT_Os: break; + case OPT_Xassembler: + /* When we detected a mismatch in assembler options between + the input TU's fall back to previous behavior of ignoring them. */ + if (xassembler_options_error) + continue; + break; + default: if (!(cl_options[option->opt_index].flags & CL_TARGET)) continue; @@ -1251,7 +1270,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 +1301,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; +} +