Refactor intelmic-mkoffload.c argv building to use obstacks (was: [PATCH 1/4] Add mkoffload for Intel MIC)

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 10359 bytes --]

Hi!

On Mon, 28 Sep 2015 13:27:32 +0200, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 09/28/2015 01:25 PM, Ilya Verbin wrote:
> > On Mon, Sep 28, 2015 at 12:09:19 +0200, Bernd Schmidt wrote:
> >> On 09/28/2015 12:03 PM, Bernd Schmidt wrote:
> >>> On 09/28/2015 10:26 AM, Thomas Schwinge wrote:
> >>>> -  objcopy_argv[8] = NULL;
> >>>> +  objcopy_argv[objcopy_argc++] = NULL;
> >>>> +  gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX);
> >>>
> >>> On its own this is not an improvement - you're trading a compile time
> >>> error for a runtime error. So, what is the other change this is
> >>> preparing for?
> >>
> >> Ok, I now see the other patch. But I also see that other code in the same
> >> file and in the nvptx mkoffload is using the obstack_ptr_grow method to
> >> build argv arrays, I think that would be preferrable to this.
> >
> > I've removed obstack_ptr_grow for arrays with known sizes after this review:
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02210.html
> 
> That's unfortunate, I think that made the code less future-proof. IMO we 
> should revert to the obstack method especially if Thomas -v patch goes in.

Given that the discussion has settled in favor of using obstacks, I have
committed the following in r228300:

commit 99043644772bdc4b76d44058014d664ce27867f7
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Sep 30 16:42:22 2015 +0000

    Refactor intelmic-mkoffload.c argv building to use obstacks
    
    That is, restore and adapt the code as originally proposed.
    
    	gcc/
    	* config/i386/intelmic-mkoffload.c (generate_host_descr_file)
    	(prepare_target_image, main): Refactor argv building to use
    	obstacks.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@228300 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog                        |   8 +++
 gcc/config/i386/intelmic-mkoffload.c | 132 +++++++++++++++++++----------------
 2 files changed, 80 insertions(+), 60 deletions(-)

diff --git gcc/ChangeLog gcc/ChangeLog
index 86f81d6..15f84ab 100644
--- gcc/ChangeLog
+++ gcc/ChangeLog
@@ -1,3 +1,11 @@
+2015-09-30  Thomas Schwinge  <thomas@codesourcery.com>
+	    Ilya Verbin  <ilya.verbin@intel.com>
+	    Andrey Turetskiy  <andrey.turetskiy@intel.com>
+
+	* config/i386/intelmic-mkoffload.c (generate_host_descr_file)
+	(prepare_target_image, main): Refactor argv building to use
+	obstacks.
+
 2015-09-30  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>
 
 	* config/spu/spu-protos.h (spu_expand_atomic_op): Add prototype.
diff --git gcc/config/i386/intelmic-mkoffload.c gcc/config/i386/intelmic-mkoffload.c
index 065d408..ae88ecd 100644
--- gcc/config/i386/intelmic-mkoffload.c
+++ gcc/config/i386/intelmic-mkoffload.c
@@ -379,29 +379,31 @@ generate_host_descr_file (const char *host_compiler)
 
   fclose (src_file);
 
-  unsigned new_argc = 0;
-  const char *new_argv[9];
-  new_argv[new_argc++] = host_compiler;
-  new_argv[new_argc++] = "-c";
-  new_argv[new_argc++] = "-fPIC";
-  new_argv[new_argc++] = "-shared";
+  struct obstack argv_obstack;
+  obstack_init (&argv_obstack);
+  obstack_ptr_grow (&argv_obstack, host_compiler);
+  obstack_ptr_grow (&argv_obstack, "-c");
+  obstack_ptr_grow (&argv_obstack, "-fPIC");
+  obstack_ptr_grow (&argv_obstack, "-shared");
   switch (offload_abi)
     {
     case OFFLOAD_ABI_LP64:
-      new_argv[new_argc++] = "-m64";
+      obstack_ptr_grow (&argv_obstack, "-m64");
       break;
     case OFFLOAD_ABI_ILP32:
-      new_argv[new_argc++] = "-m32";
+      obstack_ptr_grow (&argv_obstack, "-m32");
       break;
     default:
       gcc_unreachable ();
     }
-  new_argv[new_argc++] = src_filename;
-  new_argv[new_argc++] = "-o";
-  new_argv[new_argc++] = obj_filename;
-  new_argv[new_argc++] = NULL;
+  obstack_ptr_grow (&argv_obstack, src_filename);
+  obstack_ptr_grow (&argv_obstack, "-o");
+  obstack_ptr_grow (&argv_obstack, obj_filename);
+  obstack_ptr_grow (&argv_obstack, NULL);
 
-  fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false);
+  char **argv = XOBFINISH (&argv_obstack, char **);
+  fork_execute (argv[0], argv, false);
+  obstack_free (&argv_obstack, NULL);
 
   return obj_filename;
 }
@@ -448,29 +450,31 @@ prepare_target_image (const char *target_compiler, int argc, char **argv)
   char *rename_section_opt
     = XALLOCAVEC (char, sizeof (".data=") + strlen (image_section_name));
   sprintf (rename_section_opt, ".data=%s", image_section_name);
-  const char *objcopy_argv[11];
-  objcopy_argv[0] = "objcopy";
-  objcopy_argv[1] = "-B";
-  objcopy_argv[2] = "i386";
-  objcopy_argv[3] = "-I";
-  objcopy_argv[4] = "binary";
-  objcopy_argv[5] = "-O";
+  obstack_init (&argv_obstack);
+  obstack_ptr_grow (&argv_obstack, "objcopy");
+  obstack_ptr_grow (&argv_obstack, "-B");
+  obstack_ptr_grow (&argv_obstack, "i386");
+  obstack_ptr_grow (&argv_obstack, "-I");
+  obstack_ptr_grow (&argv_obstack, "binary");
+  obstack_ptr_grow (&argv_obstack, "-O");
   switch (offload_abi)
     {
     case OFFLOAD_ABI_LP64:
-      objcopy_argv[6] = "elf64-x86-64";
+      obstack_ptr_grow (&argv_obstack, "elf64-x86-64");
       break;
     case OFFLOAD_ABI_ILP32:
-      objcopy_argv[6] = "elf32-i386";
+      obstack_ptr_grow (&argv_obstack, "elf32-i386");
       break;
     default:
       gcc_unreachable ();
     }
-  objcopy_argv[7] = target_so_filename;
-  objcopy_argv[8] = "--rename-section";
-  objcopy_argv[9] = rename_section_opt;
-  objcopy_argv[10] = NULL;
-  fork_execute (objcopy_argv[0], CONST_CAST (char **, objcopy_argv), false);
+  obstack_ptr_grow (&argv_obstack, target_so_filename);
+  obstack_ptr_grow (&argv_obstack, "--rename-section");
+  obstack_ptr_grow (&argv_obstack, rename_section_opt);
+  obstack_ptr_grow (&argv_obstack, NULL);
+  char **new_argv = XOBFINISH (&argv_obstack, char **);
+  fork_execute (new_argv[0], new_argv, false);
+  obstack_free (&argv_obstack, NULL);
 
   /* Objcopy has created symbols, containing the input file name with
      non-alphanumeric characters replaced by underscores.
@@ -500,16 +504,19 @@ prepare_target_image (const char *target_compiler, int argc, char **argv)
   sprintf (opt_for_objcopy[1], "_binary_%s_end=%s", symbol_name, symbols[1]);
   sprintf (opt_for_objcopy[2], "_binary_%s_size=%s", symbol_name, symbols[2]);
 
-  objcopy_argv[0] = "objcopy";
-  objcopy_argv[1] = target_so_filename;
-  objcopy_argv[2] = "--redefine-sym";
-  objcopy_argv[3] = opt_for_objcopy[0];
-  objcopy_argv[4] = "--redefine-sym";
-  objcopy_argv[5] = opt_for_objcopy[1];
-  objcopy_argv[6] = "--redefine-sym";
-  objcopy_argv[7] = opt_for_objcopy[2];
-  objcopy_argv[8] = NULL;
-  fork_execute (objcopy_argv[0], CONST_CAST (char **, objcopy_argv), false);
+  obstack_init (&argv_obstack);
+  obstack_ptr_grow (&argv_obstack, "objcopy");
+  obstack_ptr_grow (&argv_obstack, target_so_filename);
+  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
+  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[0]);
+  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
+  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[1]);
+  obstack_ptr_grow (&argv_obstack, "--redefine-sym");
+  obstack_ptr_grow (&argv_obstack, opt_for_objcopy[2]);
+  obstack_ptr_grow (&argv_obstack, NULL);
+  new_argv = XOBFINISH (&argv_obstack, char **);
+  fork_execute (new_argv[0], new_argv, false);
+  obstack_free (&argv_obstack, NULL);
 
   return target_so_filename;
 }
@@ -562,41 +569,46 @@ main (int argc, char **argv)
 
   /* Perform partial linking for the target image and host side descriptor.
      As a result we'll get a finalized object file with all offload data.  */
-  unsigned new_argc = 0;
-  const char *new_argv[9];
-  new_argv[new_argc++] = "ld";
-  new_argv[new_argc++] = "-m";
+  struct obstack argv_obstack;
+  obstack_init (&argv_obstack);
+  obstack_ptr_grow (&argv_obstack, "ld");
+  obstack_ptr_grow (&argv_obstack, "-m");
   switch (offload_abi)
     {
     case OFFLOAD_ABI_LP64:
-      new_argv[new_argc++] = "elf_x86_64";
+      obstack_ptr_grow (&argv_obstack, "elf_x86_64");
       break;
     case OFFLOAD_ABI_ILP32:
-      new_argv[new_argc++] = "elf_i386";
+      obstack_ptr_grow (&argv_obstack, "elf_i386");
       break;
     default:
       gcc_unreachable ();
     }
-  new_argv[new_argc++] = "--relocatable";
-  new_argv[new_argc++] = host_descr_filename;
-  new_argv[new_argc++] = target_so_filename;
-  new_argv[new_argc++] = "-o";
-  new_argv[new_argc++] = out_obj_filename;
-  new_argv[new_argc++] = NULL;
-  fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false);
+  obstack_ptr_grow (&argv_obstack, "--relocatable");
+  obstack_ptr_grow (&argv_obstack, host_descr_filename);
+  obstack_ptr_grow (&argv_obstack, target_so_filename);
+  obstack_ptr_grow (&argv_obstack, "-o");
+  obstack_ptr_grow (&argv_obstack, out_obj_filename);
+  obstack_ptr_grow (&argv_obstack, NULL);
+  char **new_argv = XOBFINISH (&argv_obstack, char **);
+  fork_execute (new_argv[0], new_argv, false);
+  obstack_free (&argv_obstack, NULL);
 
   /* Run objcopy on the resultant object file to localize generated symbols
      to avoid conflicting between different DSO and an executable.  */
-  new_argv[0] = "objcopy";
-  new_argv[1] = "-L";
-  new_argv[2] = symbols[0];
-  new_argv[3] = "-L";
-  new_argv[4] = symbols[1];
-  new_argv[5] = "-L";
-  new_argv[6] = symbols[2];
-  new_argv[7] = out_obj_filename;
-  new_argv[8] = NULL;
-  fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false);
+  obstack_init (&argv_obstack);
+  obstack_ptr_grow (&argv_obstack, "objcopy");
+  obstack_ptr_grow (&argv_obstack, "-L");
+  obstack_ptr_grow (&argv_obstack, symbols[0]);
+  obstack_ptr_grow (&argv_obstack, "-L");
+  obstack_ptr_grow (&argv_obstack, symbols[1]);
+  obstack_ptr_grow (&argv_obstack, "-L");
+  obstack_ptr_grow (&argv_obstack, symbols[2]);
+  obstack_ptr_grow (&argv_obstack, out_obj_filename);
+  obstack_ptr_grow (&argv_obstack, NULL);
+  new_argv = XOBFINISH (&argv_obstack, char **);
+  fork_execute (new_argv[0], new_argv, false);
+  obstack_free (&argv_obstack, NULL);
 
   return 0;
 }


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]