RE: [RFC] New feature to reuse one multilib among different targets
> -Original Message- > From: Joseph Myers [mailto:jos...@codesourcery.com] > Sent: Tuesday, January 08, 2013 12:13 AM > To: Terry Guo > Cc: gcc-patches@gcc.gnu.org > Subject: RE: [RFC] New feature to reuse one multilib among different targets > > On Fri, 7 Dec 2012, Terry Guo wrote: > > > 2012-12-07 Terry Guo > > > > * gcc/Makefile.in (s-mlib): New argument MULTILIB_REUSE. > > * gcc/doc/fragments.texi: Document MULTILIB_REUSE. > > * gcc/gcc.c (multilib_reuse): New internal spec. > > (set_multilib_dir): Also search multilib from multilib_reuse. > > * gcc/genmultilib (tmpmultilib3): Refactor code. > > (tmpmultilib4): Ditto. > > (multilib_reuse): New multilib argument. > > This patch is OK. > Thanks very much for review. Since there is no objection over the past days, I just committed those patches to trunk. BR, Terry
RE: [RFC] New feature to reuse one multilib among different targets
On Fri, 7 Dec 2012, Terry Guo wrote: > 2012-12-07 Terry Guo > > * gcc/Makefile.in (s-mlib): New argument MULTILIB_REUSE. > * gcc/doc/fragments.texi: Document MULTILIB_REUSE. > * gcc/gcc.c (multilib_reuse): New internal spec. > (set_multilib_dir): Also search multilib from multilib_reuse. > * gcc/genmultilib (tmpmultilib3): Refactor code. > (tmpmultilib4): Ditto. > (multilib_reuse): New multilib argument. This patch is OK. -- Joseph S. Myers jos...@codesourcery.com
RE: [RFC] New feature to reuse one multilib among different targets
> -Original Message-
> From: Joseph Myers [mailto:jos...@codesourcery.com]
> Sent: Friday, December 07, 2012 2:04 AM
> To: Terry Guo
> Cc: gcc-patches@gcc.gnu.org
> Subject: RE: [RFC] New feature to reuse one multilib among different
> targets
>
> On Tue, 13 Nov 2012, Terry Guo wrote:
>
> > +@findex MULTILIB_REUSE
> > +@item MULTILIB_REUSE
> > +Sometimes it is desirable to reuse one existing multilib among
> different
> > +targets. Such kind of reuse can minimize the number of multilib
> variants.
>
> I don't think "among different targets" is the right wording here.
> "for
> different sets of options"?
>
Updated with your comments.
> > +A typical reuse rule is comprised of two parts connected by equality
> sign.
>
> Not just a typical reuse rule, but *any* reuse rule, as I understand it.
>
You are right. Removed the word "typical".
> > +The left part of the rule are the options used to build multilib and
> the right
> > +part are the options representing target that will reuse this
> multilib. The
> > +equality sign in both parts should be replaced with period.
>
> Is the order of the options significant, on either or both sides? Can
> the
> options on the right hand side be options that aren't used to build any
> multilibs? I think the documentation should answer that sort of
> question.
>
Yes, the option order in left part matters. More explanations are added as
in patch.
> > @@ -7475,10 +7484,16 @@ set_multilib_dir (void)
> >
> >first = 1;
> >p = multilib_select;
> > +
> > + /* Append multilib reuse rules if any. With those rules, we can
> reuse
> > + one multilib for certain different targets. */
> > + if (strlen(multilib_reuse) > 0)
>
> Missing space before '('.
>
Added the missing space.
> > - /* Ignore newlines. */
> > - if (*p == '\n')
> > + /* Ignore newlinesi and spaces. */
>
> Typo "newlinesi". And why the change to ignore spaces as well - what's
> different about this new feature to require that?
>
Typo is corrected. I once wanted to enable user to change multilib spec in
gcc driver on the fly by defining own spec file with content:
*multilib:
OWN RULES
or
*multilib:
+ OWN RULES
For the latter case, an extra space will be involved and break the parse of
multilib spec. So I ignore space here.
But as your said we had better not touch those internal data structure, I
give up this idea now.
> > @@ -7491,8 +7506,8 @@ set_multilib_dir (void)
> > if (*p == '\0')
> > {
> > invalid_select:
> > - fatal_error ("multilib select %qs is invalid",
> > - multilib_select);
> > + fatal_error ("multilib select %qs%qs is invalid",
> > + multilib_select, multilib_reuse);
>
> Printing two quoted strings with no space between the closing quote of
> one
> and the opening quote of the other certainly doesn't seem right.
> (Really
> this whole error message seems pretty bad - it won't make sense to
> users -
> but that's a pre-existing condition.)
>
An extra space is inserted.
> > +rm -rf tmpmultilib3
> > +cat >tmpmultilib3 <<\EOF
>
> As I understand it, this is a refactoring of existing code. The patch
> might be easier to review if the bits that just refactored existing
> code
> into sub-scripts (without any changes to that code) were sent as a
> separate self-contained patch, and then the new feature patch was sent
> as
> a patch applying on top of those.
>
Your understanding is correct. Now I put code refactor part into patch 00.
Patch 01 is supposed to be applied on top of it.
> > + # We only care rule that has concrete multilib.
>
> "care about", I think, but this sentence still doesn't really make
> sense
> to me. What are the cases that aren't being cared about here, and why
> are
> they valid inputs? Surely, given a proper MULTILIB_REUSE setting,
> every
> rule in that setting should do something meaningful and rules that
> don't
> should result in errors?
>
Now an error will be generated once the rule tries to reuse nonexistent
multilib.
Thank you again, Joseph.
BR,
Terry
2012-12-07 Terry Guo
* gcc/Makefile.in (s-mlib): New argument MULTILIB_REUSE.
* gcc/doc/fragments.texi: Document MULTILIB_REUSE.
* gcc/gcc.c (multilib_reuse): New internal spec.
(set_multilib_dir): Also search multilib from multilib_reuse.
* gcc/genmultilib (tmpmultilib3): Refactor code.
(tmpmultilib4): Ditto.
(multilib_reuse): New multilib argument.
00-multilib-reuse-v5.patch
Description: Binary data
01-multilib-reuse-v5.patch
Description: Binary data
RE: [RFC] New feature to reuse one multilib among different targets
On Tue, 13 Nov 2012, Terry Guo wrote:
> +@findex MULTILIB_REUSE
> +@item MULTILIB_REUSE
> +Sometimes it is desirable to reuse one existing multilib among different
> +targets. Such kind of reuse can minimize the number of multilib variants.
I don't think "among different targets" is the right wording here. "for
different sets of options"?
> +A typical reuse rule is comprised of two parts connected by equality sign.
Not just a typical reuse rule, but *any* reuse rule, as I understand it.
> +The left part of the rule are the options used to build multilib and the
> right
> +part are the options representing target that will reuse this multilib. The
> +equality sign in both parts should be replaced with period.
Is the order of the options significant, on either or both sides? Can the
options on the right hand side be options that aren't used to build any
multilibs? I think the documentation should answer that sort of question.
> @@ -7475,10 +7484,16 @@ set_multilib_dir (void)
>
>first = 1;
>p = multilib_select;
> +
> + /* Append multilib reuse rules if any. With those rules, we can reuse
> + one multilib for certain different targets. */
> + if (strlen(multilib_reuse) > 0)
Missing space before '('.
> - /* Ignore newlines. */
> - if (*p == '\n')
> + /* Ignore newlinesi and spaces. */
Typo "newlinesi". And why the change to ignore spaces as well - what's
different about this new feature to require that?
> @@ -7491,8 +7506,8 @@ set_multilib_dir (void)
> if (*p == '\0')
> {
> invalid_select:
> - fatal_error ("multilib select %qs is invalid",
> -multilib_select);
> + fatal_error ("multilib select %qs%qs is invalid",
> +multilib_select, multilib_reuse);
Printing two quoted strings with no space between the closing quote of one
and the opening quote of the other certainly doesn't seem right. (Really
this whole error message seems pretty bad - it won't make sense to users -
but that's a pre-existing condition.)
> +rm -rf tmpmultilib3
> +cat >tmpmultilib3 <<\EOF
As I understand it, this is a refactoring of existing code. The patch
might be easier to review if the bits that just refactored existing code
into sub-scripts (without any changes to that code) were sent as a
separate self-contained patch, and then the new feature patch was sent as
a patch applying on top of those.
> + # We only care rule that has concrete multilib.
"care about", I think, but this sentence still doesn't really make sense
to me. What are the cases that aren't being cared about here, and why are
they valid inputs? Surely, given a proper MULTILIB_REUSE setting, every
rule in that setting should do something meaningful and rules that don't
should result in errors?
--
Joseph S. Myers
jos...@codesourcery.com
Ping-2: RE: [RFC] New feature to reuse one multilib among different targets
Hi Joseph, Can you please review this patch? If I missed something, please point out. Thanks. BR, Terry > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Terry Guo > Sent: Friday, November 23, 2012 5:12 PM > To: jos...@codesourcery.com > Cc: gcc-patches@gcc.gnu.org > Subject: Ping: RE: [RFC] New feature to reuse one multilib among > different targets > > Hi Joseph, > > Can you please help to review this patch and share your thoughts on > this > feature? Thanks. > > BR, > Terry > > > -Original Message- > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > > ow...@gcc.gnu.org] On Behalf Of Terry Guo > > Sent: Tuesday, November 13, 2012 12:47 PM > > To: jos...@codesourcery.com > > Cc: gcc-patches@gcc.gnu.org > > Subject: RE: [RFC] New feature to reuse one multilib among different > > targets > > > > > -Original Message- > > > From: Joseph Myers [mailto:jos...@codesourcery.com] > > > Sent: Saturday, November 10, 2012 12:35 AM > > > To: Terry Guo > > > Cc: gcc-patches@gcc.gnu.org > > > Subject: RE: [RFC] New feature to reuse one multilib among > different > > > targets > > > > > > On Fri, 9 Nov 2012, Terry Guo wrote: > > > > > > > You are right that we should make script POSIX compliant. This v3 > > > patch > > > > removed "function" and "local" which don't belong to POSIX > standard. > > > I also > > > > verified that CONFIG_SHELL is passed to this script with value > > > "/bin/sh". > > > > > > Suppose /bin/sh is not a POSIX shell but the user sets CONFIG_SHELL > > to > > > something else (which is a POSIX shell). Will SHELL in the > makefile > > > get set to the POSIX shell the user specified as CONFIG_SHELL? > > That's > > > what's needed to be able to use POSIX shell features in this script. > > > > > > > The attached patch is updated to use sub-script rather than the > > function to > > reuse code. Is it ok to avoid the issue you just mentioned? > > > > BR, > > Terry > > > > 2012-11-13 Terry Guo > > > > * genmultilib (tmpmultilib3): New refactored sub-script > > to convert the option combination into folder name. > > (tmpmultilib4): New refactored sub-script to output the > > options in a option combination. > > (MULTILIB_REUSE): New argument. > > * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. > > * gcc.c (multilib_reuse): New spec. > > (set_multilib_dir): Use multilib_reuse. > > * doc/fragments.texi: Mention MULTILIB_REUSE. > >
Ping: RE: [RFC] New feature to reuse one multilib among different targets
Hi Joseph, Can you please help to review this patch and share your thoughts on this feature? Thanks. BR, Terry > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Terry Guo > Sent: Tuesday, November 13, 2012 12:47 PM > To: jos...@codesourcery.com > Cc: gcc-patches@gcc.gnu.org > Subject: RE: [RFC] New feature to reuse one multilib among different > targets > > > -Original Message- > > From: Joseph Myers [mailto:jos...@codesourcery.com] > > Sent: Saturday, November 10, 2012 12:35 AM > > To: Terry Guo > > Cc: gcc-patches@gcc.gnu.org > > Subject: RE: [RFC] New feature to reuse one multilib among different > > targets > > > > On Fri, 9 Nov 2012, Terry Guo wrote: > > > > > You are right that we should make script POSIX compliant. This v3 > > patch > > > removed "function" and "local" which don't belong to POSIX standard. > > I also > > > verified that CONFIG_SHELL is passed to this script with value > > "/bin/sh". > > > > Suppose /bin/sh is not a POSIX shell but the user sets CONFIG_SHELL > to > > something else (which is a POSIX shell). Will SHELL in the makefile > > get set to the POSIX shell the user specified as CONFIG_SHELL? > That's > > what's needed to be able to use POSIX shell features in this script. > > > > The attached patch is updated to use sub-script rather than the > function to > reuse code. Is it ok to avoid the issue you just mentioned? > > BR, > Terry > > 2012-11-13 Terry Guo > > * genmultilib (tmpmultilib3): New refactored sub-script > to convert the option combination into folder name. > (tmpmultilib4): New refactored sub-script to output the > options in a option combination. > (MULTILIB_REUSE): New argument. > * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. > * gcc.c (multilib_reuse): New spec. > (set_multilib_dir): Use multilib_reuse. > * doc/fragments.texi: Mention MULTILIB_REUSE.
RE: [RFC] New feature to reuse one multilib among different targets
> -Original Message- > From: Joseph Myers [mailto:jos...@codesourcery.com] > Sent: Saturday, November 10, 2012 12:35 AM > To: Terry Guo > Cc: gcc-patches@gcc.gnu.org > Subject: RE: [RFC] New feature to reuse one multilib among different > targets > > On Fri, 9 Nov 2012, Terry Guo wrote: > > > You are right that we should make script POSIX compliant. This v3 > patch > > removed "function" and "local" which don't belong to POSIX standard. > I also > > verified that CONFIG_SHELL is passed to this script with value > "/bin/sh". > > Suppose /bin/sh is not a POSIX shell but the user sets CONFIG_SHELL to > something else (which is a POSIX shell). Will SHELL in the makefile > get > set to the POSIX shell the user specified as CONFIG_SHELL? That's > what's > needed to be able to use POSIX shell features in this script. > The attached patch is updated to use sub-script rather than the function to reuse code. Is it ok to avoid the issue you just mentioned? BR, Terry 2012-11-13 Terry Guo * genmultilib (tmpmultilib3): New refactored sub-script to convert the option combination into folder name. (tmpmultilib4): New refactored sub-script to output the options in a option combination. (MULTILIB_REUSE): New argument. * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. * gcc.c (multilib_reuse): New spec. (set_multilib_dir): Use multilib_reuse. * doc/fragments.texi: Mention MULTILIB_REUSE. multilib-reuse-v4.patch Description: Binary data
RE: [RFC] New feature to reuse one multilib among different targets
On Fri, 9 Nov 2012, Terry Guo wrote: > You are right that we should make script POSIX compliant. This v3 patch > removed "function" and "local" which don't belong to POSIX standard. I also > verified that CONFIG_SHELL is passed to this script with value "/bin/sh". Suppose /bin/sh is not a POSIX shell but the user sets CONFIG_SHELL to something else (which is a POSIX shell). Will SHELL in the makefile get set to the POSIX shell the user specified as CONFIG_SHELL? That's what's needed to be able to use POSIX shell features in this script. -- Joseph S. Myers jos...@codesourcery.com
RE: [RFC] New feature to reuse one multilib among different targets
> -Original Message- > From: Joseph Myers [mailto:jos...@codesourcery.com] > Sent: Friday, November 09, 2012 5:10 AM > To: Terry Guo > Cc: gcc-patches@gcc.gnu.org > Subject: RE: [RFC] New feature to reuse one multilib among different > targets > > On Thu, 8 Nov 2012, Terry Guo wrote: > > > To convert such statements to data structure used by multilib_raw, I > > refactor codes in genmultilib into two functions combo_to_dir and > > The "function" keyword for creating shell functions is not POSIX, and I > don't know if we ensure that $SHELL is a shell supporting functions. > (It's documented that CONFIG_SHELL may need to be set to a POSIX shell > if /bin/sh isn't sufficient, but does that feed through to the value of > SHELL used to run this script?) > You are right that we should make script POSIX compliant. This v3 patch removed "function" and "local" which don't belong to POSIX standard. I also verified that CONFIG_SHELL is passed to this script with value "/bin/sh". Checked new genmultilib script with command "checkbashisms --posix genmultilib" in Ubuntu. No warning and error messages reported. [...] > > Documentation changes need mentioning in ChangeLog entries. > Added them in following ChangeLog. BR, Terry 2012-11-09 Terry Guo * genmultilib (combo_to_dir): New function. (options_output): New function. (MULTILIB_REUSE): New argument. * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. * gcc.c (multilib_reuse): New spec. (set_multilib_dir): Use multilib_reuse. * doc/fragments.texi: Mention MULTILIB_REUSE. multilib-reuse-v3.patch Description: Binary data
RE: [RFC] New feature to reuse one multilib among different targets
On Thu, 8 Nov 2012, Terry Guo wrote: > To convert such statements to data structure used by multilib_raw, I > refactor codes in genmultilib into two functions combo_to_dir and The "function" keyword for creating shell functions is not POSIX, and I don't know if we ensure that $SHELL is a shell supporting functions. (It's documented that CONFIG_SHELL may need to be set to a POSIX shell if /bin/sh isn't sufficient, but does that feed through to the value of SHELL used to run this script?) > 2012-11-08 Terry Guo > > * genmultilib (combo_to_dir): New function. > (options_output): New function. > (MULTILIB_REUSE): New argument. > * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. > * gcc.c (multilib_reuse): New spec. > (set_multilib_dir): Use multilib_reuse. Documentation changes need mentioning in ChangeLog entries. -- Joseph S. Myers jos...@codesourcery.com
RE: [RFC] New feature to reuse one multilib among different targets
[...] > > Please help to review this new Multilib feature. It intends to > provide > > user > > Your patch doesn't include documentation for fragments.texi (which > needs to define the semantics without reference to the details of what > gcc.c's internal datastructures for multilibs, as output by genmultilib, > might look like). > > I am unconvinced that directly adding to the drivers' internal > datastructures like this is a sensible interface for specifying > multilib choice in target makefile fragments. > Very appreciate your review and comments. Here is an updated patch which follows the approaches used in current multilib implementation. With this update, the following statement means target represented by "optC optD" can reuse existing multilib built by options "optA optB": MULTILIB_REUSE = optA/optB=optC/optD To convert such statements to data structure used by multilib_raw, I refactor codes in genmultilib into two functions combo_to_dir and options_output. Then use combo_to_dir to convert left part into multilib folder name and use options_output to convert right part into option list. Inside gcc.c, those reuse rules will be used once gcc can't figure out multilib that exactly matches current command line options. I build trunk code with this patch along with --enable-multilib for targets arm-none-eabi/x86/m6800/mips/powerpc. No problem found. Is this patch OK? Please comment. BR, Terry 2012-11-08 Terry Guo * genmultilib (combo_to_dir): New function. (options_output): New function. (MULTILIB_REUSE): New argument. * Makefile.in (s-mlib): Add a new argument MULTILIB_REUSE. * gcc.c (multilib_reuse): New spec. (set_multilib_dir): Use multilib_reuse. multilib-reuse-v2.patch Description: Binary data
Re: [RFC] New feature to reuse one multilib among different targets
On Wed, 10 Oct 2012, Terry Guo wrote: > Hello Joseph, > > Please help to review this new Multilib feature. It intends to provide user Your patch doesn't include documentation for fragments.texi (which needs to define the semantics without reference to the details of what gcc.c's internal datastructures for multilibs, as output by genmultilib, might look like). I am unconvinced that directly adding to the drivers' internal datastructures like this is a sensible interface for specifying multilib choice in target makefile fragments. -- Joseph S. Myers jos...@codesourcery.com
