From patchwork Wed Sep 30 16:46:54 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Thomas Schwinge X-Patchwork-Id: 524486 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 60C6C140D2E for ; Thu, 1 Oct 2015 02:47:20 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=vYYKToX1; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; q=dns; s=default; b=l7ii1UoQqMhsTjfH 2gsRskVhDXXyRkpP+s09rdvdZL09UrnC3yo+dY/W7TmcRFr2btAGS9UHUfp6dq6Z MHQXYc7X9eFP5xAkIY2JCZDB10X8JqOsOI1hsblHWs/6OXtRM9V+yhoT2WRNEX9m Ko9qGtwaKzv7JTx3nNl4g2ZaTqQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:in-reply-to:references:date:message-id :mime-version:content-type; s=default; bh=OIcVeMj9fGhWFC9YhJuSoi gOegY=; b=vYYKToX1o1BagkyMER8NTlSHi21GG5JiP/TfrtJ+xzpvMnDv/UNJKn eZH0HMZj46zzguPhe2d1yXm9pB44lb0bcPyakorpQZeJbGFG/3Xk5YTnvqcRjwKi XGdPjxeE2EXy57I/JtUMSpoQRwZU+jSbZu1qjAKdPl+O0hFovoTC8= Received: (qmail 122266 invoked by alias); 30 Sep 2015 16:47:14 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 122256 invoked by uid 89); 30 Sep 2015 16:47:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 30 Sep 2015 16:47:07 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-03.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ZhKX0-0001kg-Mg from Thomas_Schwinge@mentor.com ; Wed, 30 Sep 2015 09:47:03 -0700 Received: from feldtkeller.schwinge.homeip.net (137.202.0.76) by SVR-IES-FEM-03.mgc.mentorg.com (137.202.0.108) with Microsoft SMTP Server id 14.3.224.2; Wed, 30 Sep 2015 17:47:01 +0100 From: Thomas Schwinge To: , Bernd Schmidt , Ilya Verbin CC: Jakub Jelinek , Kirill Yukhin Subject: Refactor intelmic-mkoffload.c argv building to use obstacks (was: [PATCH 1/4] Add mkoffload for Intel MIC) In-Reply-To: <56092424.6000808@redhat.com> References: <20141021171323.GA47586@msticlxl57.ims.intel.com> <20141021171602.GB47586@msticlxl57.ims.intel.com> <20141022082103.GH10376@tucnak.redhat.com> <20141022185701.GA21398@msticlxl57.ims.intel.com> <87r3ljuez8.fsf@kepler.schwinge.homeip.net> <5609106F.9040707@redhat.com> <560911CF.2010000@redhat.com> <20150928112548.GA25312@msticlxl57.ims.intel.com> <56092424.6000808@redhat.com> User-Agent: Notmuch/0.9-125-g4686d11 (http://notmuchmail.org) Emacs/24.5.1 (i586-pc-linux-gnu) Date: Wed, 30 Sep 2015 18:46:54 +0200 Message-ID: <87h9mbsvlt.fsf@kepler.schwinge.homeip.net> MIME-Version: 1.0 Hi! On Mon, 28 Sep 2015 13:27:32 +0200, Bernd Schmidt 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 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(-) Grüße, Thomas diff --git gcc/ChangeLog gcc/ChangeLog index 86f81d6..15f84ab 100644 --- gcc/ChangeLog +++ gcc/ChangeLog @@ -1,3 +1,11 @@ +2015-09-30 Thomas Schwinge + Ilya Verbin + Andrey Turetskiy + + * config/i386/intelmic-mkoffload.c (generate_host_descr_file) + (prepare_target_image, main): Refactor argv building to use + obstacks. + 2015-09-30 Ulrich Weigand * 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; }