Hi!
On Wed, 22 Oct 2014 22:57:01 +0400, Ilya Verbin <iver...@gmail.com> wrote:
> On 22 Oct 09:57, Jakub Jelinek wrote:
> > On Wed, Oct 22, 2014 at 02:30:44AM +0400, Ilya Verbin wrote:
> > > + 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 **);
> >
> > Why do you use an obstack for an array of pointers where you know
> > you have exactly 9 pointers? Wouldn't
> > char *new_argv[9];
> > and just pointer assignments be better?
>
> Yes, done.
>
> > > + /* 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. */
> > > + struct obstack argv_obstack;
> > > + obstack_init (&argv_obstack);
> > > + obstack_ptr_grow (&argv_obstack, "ld");
> > > + if (target_ilp32)
> > > + {
> > > + obstack_ptr_grow (&argv_obstack, "-m");
> > > + obstack_ptr_grow (&argv_obstack, "elf_i386");
> > > + }
> > > + obstack_ptr_grow (&argv_obstack, "-r");
> > > + 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 **);
> >
> > Similarly (well, here it is not constant, still, you know small upper bound
> > and can just use some int index you ++ in each assignment.
>
> Done.
>
> > > + /* Run objcopy on the resultant object file to localize generated
> > > symbols
> > > + to avoid conflicting between different DSO and an executable. */
> > > + 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);
> >
> > Likewise.
>
> Done.After approval for "Use gcc/coretypes.h:enum offload_abi in mkoffloads", <http://news.gmane.org/find-root.php?message_id=%3C87vbavuft0.fsf%40kepler.schwinge.homeip.net%3E>, I'd like to commit the following refactoring patch to trunk, in preparation for another change: commit 91fbe15ce2e539a4017f65cc167b362a4b4e4553 Author: Thomas Schwinge <tho...@codesourcery.com> Date: Tue Aug 4 14:06:39 2015 +0200 Refactor intelmic-mkoffload.c argv building gcc/ * config/i386/intelmic-mkoffload.c (generate_host_descr_file) (prepare_target_image, main): Refactor argv building. --- gcc/config/i386/intelmic-mkoffload.c | 88 +++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 34 deletions(-) diff --git gcc/config/i386/intelmic-mkoffload.c gcc/config/i386/intelmic-mkoffload.c index 8028584..8d5af0d 100644 --- gcc/config/i386/intelmic-mkoffload.c +++ gcc/config/i386/intelmic-mkoffload.c @@ -380,7 +380,8 @@ generate_host_descr_file (const char *host_compiler) fclose (src_file); unsigned new_argc = 0; - const char *new_argv[9]; +#define NEW_ARGC_MAX 9 + const char *new_argv[NEW_ARGC_MAX]; new_argv[new_argc++] = host_compiler; new_argv[new_argc++] = "-c"; new_argv[new_argc++] = "-fPIC"; @@ -400,6 +401,8 @@ generate_host_descr_file (const char *host_compiler) new_argv[new_argc++] = "-o"; new_argv[new_argc++] = obj_filename; new_argv[new_argc++] = NULL; + gcc_checking_assert (new_argc <= NEW_ARGC_MAX); +#undef NEW_ARGC_MAX fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false); @@ -444,32 +447,37 @@ prepare_target_image (const char *target_compiler, int argc, char **argv) obstack_ptr_grow (&argv_obstack, target_so_filename); compile_for_target (&argv_obstack); + unsigned objcopy_argc; +#define OBJCOPY_ARGC_MAX 11 + const char *objcopy_argv[OBJCOPY_ARGC_MAX]; + /* Run objcopy. */ 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"; + objcopy_argc = 0; + objcopy_argv[objcopy_argc++] = "objcopy"; + objcopy_argv[objcopy_argc++] = "-B"; + objcopy_argv[objcopy_argc++] = "i386"; + objcopy_argv[objcopy_argc++] = "-I"; + objcopy_argv[objcopy_argc++] = "binary"; + objcopy_argv[objcopy_argc++] = "-O"; switch (offload_abi) { case OFFLOAD_ABI_LP64: - objcopy_argv[6] = "elf64-x86-64"; + objcopy_argv[objcopy_argc++] = "elf64-x86-64"; break; case OFFLOAD_ABI_ILP32: - objcopy_argv[6] = "elf32-i386"; + objcopy_argv[objcopy_argc++] = "elf32-i386"; break; default: abort (); } - objcopy_argv[7] = target_so_filename; - objcopy_argv[8] = "--rename-section"; - objcopy_argv[9] = rename_section_opt; - objcopy_argv[10] = NULL; + objcopy_argv[objcopy_argc++] = target_so_filename; + objcopy_argv[objcopy_argc++] = "--rename-section"; + objcopy_argv[objcopy_argc++] = rename_section_opt; + objcopy_argv[objcopy_argc++] = NULL; + gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX); fork_execute (objcopy_argv[0], CONST_CAST (char **, objcopy_argv), false); /* Objcopy has created symbols, containing the input file name with @@ -500,17 +508,21 @@ 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; + objcopy_argc = 0; + objcopy_argv[objcopy_argc++] = "objcopy"; + objcopy_argv[objcopy_argc++] = target_so_filename; + objcopy_argv[objcopy_argc++] = "--redefine-sym"; + objcopy_argv[objcopy_argc++] = opt_for_objcopy[0]; + objcopy_argv[objcopy_argc++] = "--redefine-sym"; + objcopy_argv[objcopy_argc++] = opt_for_objcopy[1]; + objcopy_argv[objcopy_argc++] = "--redefine-sym"; + objcopy_argv[objcopy_argc++] = opt_for_objcopy[2]; + objcopy_argv[objcopy_argc++] = NULL; + gcc_checking_assert (objcopy_argc <= OBJCOPY_ARGC_MAX); fork_execute (objcopy_argv[0], CONST_CAST (char **, objcopy_argv), false); +#undef OBJCOPY_ARGC_MAX + return target_so_filename; } @@ -560,10 +572,13 @@ main (int argc, char **argv) const char *host_descr_filename = generate_host_descr_file (host_compiler); + unsigned new_argc; +#define NEW_ARGC_MAX 9 + const char *new_argv[NEW_ARGC_MAX]; + /* 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_argc = 0; new_argv[new_argc++] = "ld"; new_argv[new_argc++] = "-m"; switch (offload_abi) @@ -583,20 +598,25 @@ main (int argc, char **argv) new_argv[new_argc++] = "-o"; new_argv[new_argc++] = out_obj_filename; new_argv[new_argc++] = NULL; + gcc_checking_assert (new_argc <= NEW_ARGC_MAX); fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false); /* 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; + new_argc = 0; + new_argv[new_argc++] = "objcopy"; + new_argv[new_argc++] = "-L"; + new_argv[new_argc++] = symbols[0]; + new_argv[new_argc++] = "-L"; + new_argv[new_argc++] = symbols[1]; + new_argv[new_argc++] = "-L"; + new_argv[new_argc++] = symbols[2]; + new_argv[new_argc++] = out_obj_filename; + new_argv[new_argc++] = NULL; + gcc_checking_assert (new_argc <= NEW_ARGC_MAX); fork_execute (new_argv[0], CONST_CAST (char **, new_argv), false); +#undef NEW_ARGC_MAX + return 0; } Grüße, Thomas
signature.asc
Description: PGP signature
