Message ID | 54AFA92C.8010108@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 5029 invoked by alias); 9 Jan 2015 10:11:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 4887 invoked by uid 89); 9 Jan 2015 10:11:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 09 Jan 2015 10:11:23 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t09AB0tZ006568 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 9 Jan 2015 05:11:00 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t09AArSu002697; Fri, 9 Jan 2015 05:10:54 -0500 Message-ID: <54AFA92C.8010108@redhat.com> Date: Fri, 09 Jan 2015 10:10:52 +0000 From: Pedro Alves <palves@redhat.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Chen Gang S <gang.chen@sunrus.com.cn>, sellcey@imgtec.com CC: Jan Kratochvil <jan.kratochvil@redhat.com>, "Maciej W. Rozycki" <macro@linux-mips.org>, Joel Brobecker <brobecker@adacore.com>, gdb-patches@sourceware.org, Yao Qi <yao@codesourcery.com> Subject: Re: [Patch] Fix build problem with system call in compile/compile.c References: <c2ab3326-5227-43ff-a755-35cf75e209d8@BAMAIL02.ba.imgtec.org> <20150106041615.GJ5445@adacore.com> <1420560255.15691.21.camel@ubuntu-sellcey> <20150107041351.GN5445@adacore.com> <1420655766.15691.44.camel@ubuntu-sellcey> <alpine.LFD.2.11.1501071858580.27020@eddie.linux-mips.org> <54AD8A76.9010702@redhat.com> <alpine.LFD.2.11.1501072330430.27020@eddie.linux-mips.org> <20150108211229.GA5634@host2.jankratochvil.net> <1420755123.2630.21.camel@ubuntu-sellcey> <54AF1118.2070707@redhat.com> <1420762209.2630.27.camel@ubuntu-sellcey> <54AF50E4.3080901@sunrus.com.cn> In-Reply-To: <54AF50E4.3080901@sunrus.com.cn> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit |
Commit Message
Pedro Alves
Jan. 9, 2015, 10:10 a.m. UTC
On 01/09/2015 03:54 AM, Chen Gang S wrote: > On 1/9/15 08:10, Steve Ellcey wrote: >> On Thu, 2015-01-08 at 23:22 +0000, Pedro Alves wrote: >>> On 01/08/2015 10:12 PM, Steve Ellcey wrote: >>> >>>> I haven't seen any objection to >>>> Chen's patch but I haven't seen an official approval either. >>> >>> FAOD, Chen's patch is OK. >>> >>> Thanks, >>> Pedro Alves >>> >> >> Excellent. Chen Gang, do you have write access to check in this patch >> or do you need someone to do the check in for you? >> > > Excuse me, I guess, I can not check in, welcome any other members help to > check in for me. I was going to apply it as is, but I recalled that the return of "system" is really a 'wait' status: > + if (system (zap)) > + warning (_("Could not remove temporary directory %s"), dir); so I tweaked the patch accordingly, and pushed it, as below. ------------ From: Chen Gang <gang.chen@sunrus.com.cn> Subject: [PATCH] gdb/compile/compile.c: Check return value of 'system' to avoid compiler warning Under Ubuntu 12, we need to check the return value of system(), or the compiler warns: gcc -g -O2 -I. -I../../binutils-gdb/gdb -I../../binutils-gdb/gdb/common -I../../binutils-gdb/gdb/config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I../../binutils-gdb/gdb/../include/opcode -I../../binutils-gdb/gdb/../opcodes/.. -I../../binutils-gdb/gdb/../readline/.. -I../bfd -I../../binutils-gdb/gdb/../bfd -I../../binutils-gdb/gdb/../include -I../libdecnumber -I../../binutils-gdb/gdb/../libdecnumber -I../../binutils-gdb/gdb/gnulib/import -Ibuild-gnulib/import -DTUI=1 -Wall -Wdeclaration-after-statement -Wpointer-arith -Wpointer-sign -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wmissing-prototypes -Wdeclaration-after-statement -Wempty-body -Wmissing-parameter-type -Wold-style-declaration -Wold-style-definition -Wformat-nonliteral -Werror -c -o compile.o -MT compile.o -MMD -MP -MF .deps/compile.Tpo ../../binutils-gdb/gdb/compile/compile.c ../../binutils-gdb/gdb/compile/compile.c: In function ‘do_rmdir’: ../../binutils-gdb/gdb/compile/compile.c:175:10: error: ignoring return value of ‘system’, declared with attribute warn_unused_result [-Werror=unused-result] cc1: all warnings being treated as errors make[2]: *** [compile.o] Error 1 make[2]: Leaving directory `/upstream/build-binutils-s390/gdb' make[1]: *** [all-gdb] Error 2 make[1]: Leaving directory `/upstream/build-binutils-s390' make: *** [all] Error 2 Also, 'zap' is leaking. 2015-01-09 Chen Gang <gang.chen.5i5j@gmail.com> Pedro Alves <palves@redhat.com> * compile/compile.c: Include "gdb_wait.h". (do_rmdir): Check return value, and free 'zap'. --- gdb/compile/compile.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
Comments
On 1/9/15 18:10, Pedro Alves wrote: > On 01/09/2015 03:54 AM, Chen Gang S wrote: >> >> Excuse me, I guess, I can not check in, welcome any other members help to >> check in for me. > I was going to apply it as is, but I recalled that the return > of "system" is really a 'wait' status: > >> + if (system (zap)) >> + warning (_("Could not remove temporary directory %s"), dir); > > so I tweaked the patch accordingly, and pushed it, as below. [...] > - system (zap); > + wstat = system (zap); > + if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) > + warning (_("Could not remove temporary directory %s"), dir); Oh, really it is. Thanks. Thanks.
On 1/9/15 18:50, Chen Gang S wrote: > On 1/9/15 18:10, Pedro Alves wrote: >> On 01/09/2015 03:54 AM, Chen Gang S wrote: >>> >>> Excuse me, I guess, I can not check in, welcome any other members help to >>> check in for me. >> I was going to apply it as is, but I recalled that the return >> of "system" is really a 'wait' status: >> >>> + if (system (zap)) >>> + warning (_("Could not remove temporary directory %s"), dir); >> >> so I tweaked the patch accordingly, and pushed it, as below. > [...] >> - system (zap); >> + wstat = system (zap); >> + if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) >> + warning (_("Could not remove temporary directory %s"), dir); > > Oh, really it is. Thanks. > Excuse me, I am not quite familiar with the patch apply working flow for binutils/gdb, it seems each patch can only have one 'Signed-of-by' for it (do not like Linux kernel or QEMU, can have multiple 'Signed-of-by'). For me, this patch need multiple 'Signed-of-by': I start the patch, and Pedro Alves give a very necessary improvement (or it will introduce a new bug, which is not recognized quite obviously by others). If what I said above is correct, one way maybe, apply my original patch firstly, then apply the fix patch by Pedro Alves. I am not quite sure whether this way is suitable or not, though. Thanks.
On 1/10/15 04:59, Chen Gang S wrote: > On 1/9/15 18:50, Chen Gang S wrote: >> On 1/9/15 18:10, Pedro Alves wrote: >>> On 01/09/2015 03:54 AM, Chen Gang S wrote: >>>> >>>> Excuse me, I guess, I can not check in, welcome any other members help to >>>> check in for me. >>> I was going to apply it as is, but I recalled that the return >>> of "system" is really a 'wait' status: >>> >>>> + if (system (zap)) >>>> + warning (_("Could not remove temporary directory %s"), dir); >>> >>> so I tweaked the patch accordingly, and pushed it, as below. >> [...] >>> - system (zap); >>> + wstat = system (zap); >>> + if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) >>> + warning (_("Could not remove temporary directory %s"), dir); >> >> Oh, really it is. Thanks. >> > > Excuse me, I am not quite familiar with the patch apply working flow for > binutils/gdb, it seems each patch can only have one 'Signed-of-by' for > it (do not like Linux kernel or QEMU, can have multiple 'Signed-of-by'). > > For me, this patch need multiple 'Signed-of-by': I start the patch, and > Pedro Alves give a very necessary improvement (or it will introduce a > new bug, which is not recognized quite obviously by others). > > If what I said above is correct, one way maybe, apply my original patch > firstly, then apply the fix patch by Pedro Alves. I am not quite sure > whether this way is suitable or not, though. > Oh, the patch is already applied with 2 appliers :-) Thanks.
> Excuse me, I am not quite familiar with the patch apply working flow for > binutils/gdb, it seems each patch can only have one 'Signed-of-by' for > it (do not like Linux kernel or QEMU, can have multiple 'Signed-of-by'). > > For me, this patch need multiple 'Signed-of-by': I start the patch, and > Pedro Alves give a very necessary improvement (or it will introduce a > new bug, which is not recognized quite obviously by others). I know you've answered your own question about your patch in particular, but I wanted to clarify a couple of things. About "can only have one 'Signed-off-by'": We do not use git's sign-off (at least, not at the moment). As you know, there is an approval process through this mailing list which is used instead, but who actually approves the patch is not recorded in the git commit. However, the approver is not the same thing at the author, or the group of authors. If you are the sole author of a patch, and I request a few small changes that you make, and then it gets approved, you are still the sole author of those changes. Hence your name remains the only name listed in the corresponding ChangeLog entries. There is a small exception where a reviewer contributes significant ideas towards the final patch, or even contributes pieces of it, in which case the reviewer now also becomes co-author, in which case his name gets added to the corresponding ChangeLog entries. > If what I said above is correct, one way maybe, apply my original patch > firstly, then apply the fix patch by Pedro Alves. I am not quite sure > whether this way is suitable or not, though. About that: We avoid that approach, because it introduces a commit where we know there is an issue. Although the issue is supposed to get fixed right after, it still isn't great in the context of "git bisect" for instance.
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c index 1d18bd7..ccac49d 100644 --- a/gdb/compile/compile.c +++ b/gdb/compile/compile.c @@ -37,6 +37,7 @@ #include "filestuff.h" #include "target.h" #include "osabi.h" +#include "gdb_wait.h" @@ -169,10 +170,14 @@ do_rmdir (void *arg) { const char *dir = arg; char *zap; - + int wstat; + gdb_assert (strncmp (dir, TMP_PREFIX, strlen (TMP_PREFIX)) == 0); zap = concat ("rm -rf ", dir, (char *) NULL); - system (zap); + wstat = system (zap); + if (wstat == -1 || !WIFEXITED (wstat) || WEXITSTATUS (wstat) != 0) + warning (_("Could not remove temporary directory %s"), dir); + XDELETEVEC (zap); } /* Return the name of the temporary directory to use for .o files, and