lto-wrapper: split arguments of getenv ("MAKE").

Message ID eb698439-3e7c-ac10-f921-e78d6baf6464@suse.cz
State New
Headers show
Series
  • lto-wrapper: split arguments of getenv ("MAKE").
Related show

Commit Message

Martin Liška May 6, 2020, 12:08 p.m.
Hi.

The patch splits arguments in getenv ("MAKE") so that one can use:

$ MAKE="make -s" gcc main.o -flto=16

Right now it fails due to:
[pid  6004] execve("/home/marxin/Programming/script-misc/make -s", ["make -s", "-f", "/tmp/ccNpRBlZ.mk", "-j16", "all"], 0x4b69b0 /* 82 vars */) = -1 ENOENT (No such file or directory)

I've tested the patch for lto.exp and now I'm doing a proper bootstrap.
Ready after tests?

Thanks,
Martin

gcc/ChangeLog:

2020-05-06  Martin Liska  <mliska@suse.cz>

	* lto-wrapper.c: Split arguments of MAKE environment
	variable.
---
  gcc/lto-wrapper.c | 35 ++++++++++++++++++++++++-----------
  1 file changed, 24 insertions(+), 11 deletions(-)

Comments

Martin Sebor via Gcc-patches May 6, 2020, 12:52 p.m. | #1
On Wed, May 6, 2020 at 2:08 PM Martin Liška <mliska@suse.cz> wrote:
>

> Hi.

>

> The patch splits arguments in getenv ("MAKE") so that one can use:

>

> $ MAKE="make -s" gcc main.o -flto=16

>

> Right now it fails due to:

> [pid  6004] execve("/home/marxin/Programming/script-misc/make -s", ["make -s", "-f", "/tmp/ccNpRBlZ.mk", "-j16", "all"], 0x4b69b0 /* 82 vars */) = -1 ENOENT (No such file or directory)

>

> I've tested the patch for lto.exp and now I'm doing a proper bootstrap.

> Ready after tests?


+         const char *make = getenv ("MAKE");
+         unsigned argc = 0;
+         struct obstack make_argv_obstack;
+         obstack_init (&make_argv_obstack);
+
+         if (make)
+           {
+             argv = buildargv (make);

you are overwriting the argv parameter here, I suggest to use a new
local variable here.

buildargv can fail in which case it returns NULL so I suggest
to fall back to "make" in that case.

+             for (; argv[argc]; argc++)
+               obstack_ptr_grow (&make_argv_obstack, argv[argc]);

argc is only used in this scope so declare it in the for() loop?

btw, I suggest to simply re-use argv_obstack here.

+         const char **argv = XOBFINISH (&make_argv_obstack, const char **);

you are shadowing argv here, what's wrong with using new_argv?

+
+         pex = collect_execute (argv[0], CONST_CAST (char **, argv),
                                 NULL, NULL, PEX_SEARCH, false);
-         do_wait (new_argv[0], pex);
+         do_wait (argv[0], pex);
+         obstack_free (&make_argv_obstack, NULL);

after pex is finished the argv allocated via buildargv should be freed
with freeargv

Richard.

+           }


> Thanks,

> Martin

>

> gcc/ChangeLog:

>

> 2020-05-06  Martin Liska  <mliska@suse.cz>

>

>         * lto-wrapper.c: Split arguments of MAKE environment

>         variable.

> ---

>   gcc/lto-wrapper.c | 35 ++++++++++++++++++++++++-----------

>   1 file changed, 24 insertions(+), 11 deletions(-)

>

>
Martin Liška May 6, 2020, 1:09 p.m. | #2
On 5/6/20 2:52 PM, Richard Biener wrote:
> On Wed, May 6, 2020 at 2:08 PM Martin Liška <mliska@suse.cz> wrote:

>>

>> Hi.

>>

>> The patch splits arguments in getenv ("MAKE") so that one can use:

>>

>> $ MAKE="make -s" gcc main.o -flto=16

>>

>> Right now it fails due to:

>> [pid  6004] execve("/home/marxin/Programming/script-misc/make -s", ["make -s", "-f", "/tmp/ccNpRBlZ.mk", "-j16", "all"], 0x4b69b0 /* 82 vars */) = -1 ENOENT (No such file or directory)

>>

>> I've tested the patch for lto.exp and now I'm doing a proper bootstrap.

>> Ready after tests?

> 

> +         const char *make = getenv ("MAKE");

> +         unsigned argc = 0;

> +         struct obstack make_argv_obstack;

> +         obstack_init (&make_argv_obstack);

> +

> +         if (make)

> +           {

> +             argv = buildargv (make);

> 

> you are overwriting the argv parameter here, I suggest to use a new

> local variable here.


Oh.

> 

> buildargv can fail in which case it returns NULL so I suggest

> to fall back to "make" in that case.

> 

> +             for (; argv[argc]; argc++)

> +               obstack_ptr_grow (&make_argv_obstack, argv[argc]);

> 

> argc is only used in this scope so declare it in the for() loop?

> 

> btw, I suggest to simply re-use argv_obstack here.


Done.

> 

> +         const char **argv = XOBFINISH (&make_argv_obstack, const char **);

> 

> you are shadowing argv here, what's wrong with using new_argv?


The shadowing in the function is really bad, it's a long function.

> 

> +

> +         pex = collect_execute (argv[0], CONST_CAST (char **, argv),

>                                   NULL, NULL, PEX_SEARCH, false);

> -         do_wait (new_argv[0], pex);

> +         do_wait (argv[0], pex);

> +         obstack_free (&make_argv_obstack, NULL);

> 

> after pex is finished the argv allocated via buildargv should be freed

> with freeargv


Done.

I hope it's better now?

Martin

> 

> Richard.

> 

> +           }

> 

> 

>> Thanks,

>> Martin

>>

>> gcc/ChangeLog:

>>

>> 2020-05-06  Martin Liska  <mliska@suse.cz>

>>

>>          * lto-wrapper.c: Split arguments of MAKE environment

>>          variable.

>> ---

>>    gcc/lto-wrapper.c | 35 ++++++++++++++++++++++++-----------

>>    1 file changed, 24 insertions(+), 11 deletions(-)

>>

>>
From d2ded95342f543c800215e4e9e2d66332183bf8b Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Wed, 6 May 2020 13:59:23 +0200
Subject: [PATCH] lto-wrapper: split arguments of getenv ("MAKE").

gcc/ChangeLog:

2020-05-06  Martin Liska  <mliska@suse.cz>

	* lto-wrapper.c: Split arguments of MAKE environment
	variable.
---
 gcc/lto-wrapper.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 19d0c224dad..e34b6979d1f 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1894,23 +1894,32 @@ cont:
 	      putenv (xstrdup ("MAKEFLAGS="));
 	      putenv (xstrdup ("MFLAGS="));
 	    }
-	  new_argv[0] = getenv ("MAKE");
-	  if (!new_argv[0])
-	    new_argv[0] = "make";
-	  new_argv[1] = "-f";
-	  new_argv[2] = makefile;
-	  i = 3;
+
+	  char **make_argv = buildargv (getenv ("MAKE"));
+	  if (make_argv)
+	    {
+	      for (unsigned argc = 0; make_argv[argc]; argc++)
+		obstack_ptr_grow (&argv_obstack, make_argv[argc]);
+	    }
+	  else
+	    obstack_ptr_grow (&argv_obstack, "make");
+
+	  obstack_ptr_grow (&argv_obstack, "-f");
+	  obstack_ptr_grow (&argv_obstack, makefile);
 	  if (!jobserver)
 	    {
 	      snprintf (jobs, 31, "-j%ld",
 			auto_parallel ? nthreads_var : parallel);
-	      new_argv[i++] = jobs;
+	      obstack_ptr_grow (&argv_obstack, jobs);
 	    }
-	  new_argv[i++] = "all";
-	  new_argv[i++] = NULL;
+	  obstack_ptr_grow (&argv_obstack, "all");
+	  obstack_ptr_grow (&argv_obstack, NULL);
+	  new_argv = XOBFINISH (&argv_obstack, const char **);
+
 	  pex = collect_execute (new_argv[0], CONST_CAST (char **, new_argv),
 				 NULL, NULL, PEX_SEARCH, false);
 	  do_wait (new_argv[0], pex);
+	  freeargv (make_argv);
 	  maybe_unlink (makefile);
 	  makefile = NULL;
 	  for (i = 0; i < nr; ++i)
-- 
2.26.2
Martin Sebor via Gcc-patches May 6, 2020, 4:42 p.m. | #3
On Wed, May 6, 2020 at 3:09 PM Martin Liška <mliska@suse.cz> wrote:
>

> On 5/6/20 2:52 PM, Richard Biener wrote:

> > On Wed, May 6, 2020 at 2:08 PM Martin Liška <mliska@suse.cz> wrote:

> >>

> >> Hi.

> >>

> >> The patch splits arguments in getenv ("MAKE") so that one can use:

> >>

> >> $ MAKE="make -s" gcc main.o -flto=16

> >>

> >> Right now it fails due to:

> >> [pid  6004] execve("/home/marxin/Programming/script-misc/make -s", ["make -s", "-f", "/tmp/ccNpRBlZ.mk", "-j16", "all"], 0x4b69b0 /* 82 vars */) = -1 ENOENT (No such file or directory)

> >>

> >> I've tested the patch for lto.exp and now I'm doing a proper bootstrap.

> >> Ready after tests?

> >

> > +         const char *make = getenv ("MAKE");

> > +         unsigned argc = 0;

> > +         struct obstack make_argv_obstack;

> > +         obstack_init (&make_argv_obstack);

> > +

> > +         if (make)

> > +           {

> > +             argv = buildargv (make);

> >

> > you are overwriting the argv parameter here, I suggest to use a new

> > local variable here.

>

> Oh.

>

> >

> > buildargv can fail in which case it returns NULL so I suggest

> > to fall back to "make" in that case.

> >

> > +             for (; argv[argc]; argc++)

> > +               obstack_ptr_grow (&make_argv_obstack, argv[argc]);

> >

> > argc is only used in this scope so declare it in the for() loop?

> >

> > btw, I suggest to simply re-use argv_obstack here.

>

> Done.

>

> >

> > +         const char **argv = XOBFINISH (&make_argv_obstack, const char **);

> >

> > you are shadowing argv here, what's wrong with using new_argv?

>

> The shadowing in the function is really bad, it's a long function.

>

> >

> > +

> > +         pex = collect_execute (argv[0], CONST_CAST (char **, argv),

> >                                   NULL, NULL, PEX_SEARCH, false);

> > -         do_wait (new_argv[0], pex);

> > +         do_wait (argv[0], pex);

> > +         obstack_free (&make_argv_obstack, NULL);

> >

> > after pex is finished the argv allocated via buildargv should be freed

> > with freeargv

>

> Done.

>

> I hope it's better now?


Perfect if you checked freeargv is happy with a NULL argument!

OK.

Thanks,
Richard.

> Martin

>

> >

> > Richard.

> >

> > +           }

> >

> >

> >> Thanks,

> >> Martin

> >>

> >> gcc/ChangeLog:

> >>

> >> 2020-05-06  Martin Liska  <mliska@suse.cz>

> >>

> >>          * lto-wrapper.c: Split arguments of MAKE environment

> >>          variable.

> >> ---

> >>    gcc/lto-wrapper.c | 35 ++++++++++++++++++++++++-----------

> >>    1 file changed, 24 insertions(+), 11 deletions(-)

> >>

> >>

>
Martin Liška May 7, 2020, 7:37 a.m. | #4
On 5/6/20 6:42 PM, Richard Biener wrote:
> Perfect if you checked freeargv is happy with a NULL argument!


Yes, it is.

Martin

Patch

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 19d0c224dad..16d85625377 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1894,23 +1894,36 @@  cont:
 	      putenv (xstrdup ("MAKEFLAGS="));
 	      putenv (xstrdup ("MFLAGS="));
 	    }
-	  new_argv[0] = getenv ("MAKE");
-	  if (!new_argv[0])
-	    new_argv[0] = "make";
-	  new_argv[1] = "-f";
-	  new_argv[2] = makefile;
-	  i = 3;
+	  const char *make = getenv ("MAKE");
+	  unsigned argc = 0;
+	  struct obstack make_argv_obstack;
+	  obstack_init (&make_argv_obstack);
+
+	  if (make)
+	    {
+	      argv = buildargv (make);
+	      for (; argv[argc]; argc++)
+		obstack_ptr_grow (&make_argv_obstack, argv[argc]);
+	    }
+	  else
+	    obstack_ptr_grow (&make_argv_obstack, "make");
+
+	  obstack_ptr_grow (&make_argv_obstack, "-f");
+	  obstack_ptr_grow (&make_argv_obstack, makefile);
 	  if (!jobserver)
 	    {
 	      snprintf (jobs, 31, "-j%ld",
 			auto_parallel ? nthreads_var : parallel);
-	      new_argv[i++] = jobs;
+	      obstack_ptr_grow (&make_argv_obstack, jobs);
 	    }
-	  new_argv[i++] = "all";
-	  new_argv[i++] = NULL;
-	  pex = collect_execute (new_argv[0], CONST_CAST (char **, new_argv),
+	  obstack_ptr_grow (&make_argv_obstack, "all");
+	  obstack_ptr_grow (&make_argv_obstack, NULL);
+	  const char **argv = XOBFINISH (&make_argv_obstack, const char **);
+
+	  pex = collect_execute (argv[0], CONST_CAST (char **, argv),
 				 NULL, NULL, PEX_SEARCH, false);
-	  do_wait (new_argv[0], pex);
+	  do_wait (argv[0], pex);
+	  obstack_free (&make_argv_obstack, NULL);
 	  maybe_unlink (makefile);
 	  makefile = NULL;
 	  for (i = 0; i < nr; ++i)