Message ID | c2ab3326-5227-43ff-a755-35cf75e209d8@BAMAIL02.ba.imgtec.org |
---|---|
State | New, archived |
Headers |
Received: (qmail 32290 invoked by alias); 6 Jan 2015 00:44:29 -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 32279 invoked by uid 89); 6 Jan 2015 00:44:28 -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_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 06 Jan 2015 00:44:27 +0000 Received: from KLMAIL01.kl.imgtec.org (unknown [192.168.5.35]) by Websense Email Security Gateway with ESMTPS id 824D948E8CEC2 for <gdb-patches@sourceware.org>; Tue, 6 Jan 2015 00:44:20 +0000 (GMT) Received: from BAMAIL02.ba.imgtec.org (10.20.40.28) by KLMAIL01.kl.imgtec.org (192.168.5.35) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 6 Jan 2015 00:44:24 +0000 Received: from ubuntu-sellcey.mips.com (192.168.65.53) by bamail02.ba.imgtec.org (10.20.40.28) with Microsoft SMTP Server id 14.3.174.1; Mon, 5 Jan 2015 16:44:21 -0800 Received: by ubuntu-sellcey.mips.com (sSMTP sendmail emulation); Mon, 05 Jan 2015 16:44:21 -0800 From: "Steve Ellcey " <sellcey@imgtec.com> Date: Mon, 5 Jan 2015 16:44:21 -0800 To: <gdb-patches@sourceware.org> Subject: [Patch] Fix build problem with system call in compile/compile.c User-Agent: Heirloom mailx 12.5 6/20/10 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-ID: <c2ab3326-5227-43ff-a755-35cf75e209d8@BAMAIL02.ba.imgtec.org> |
Commit Message
Steve Ellcey
Jan. 6, 2015, 12:44 a.m. UTC
I am building gdb (and all of binutils) on ubuntu 12.04 with GCC 4.6.3. During compilation the gdb build fails with: /scratch/sellcey/repos/nightly_test/src/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[1]: *** [compile.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make[1]: Leaving directory `/scratch/sellcey/repos/nightly_test/obj-mips-img-linux-gnu/binutils-gdb/gdb' make: *** [all-gdb] Error 2 I am not sure why other people aren't running into this but I would like to apply this patch to fix the build problem. OK to checkin? Steve Ellcey sellcey@imgtec.com 2015-01-05 Steve Ellcey <sellcey@imgtec.com> * compile/compile.c (do_rmdir): Assign return value of system call.
Comments
"Steve Ellcey " <sellcey@imgtec.com> writes: Hi, Steve, Thanks for fixing this build failure... > /scratch/sellcey/repos/nightly_test/src/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[1]: *** [compile.o] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make[1]: Leaving directory `/scratch/sellcey/repos/nightly_test/obj-mips-img-linux-gnu/binutils-gdb/gdb' > make: *** [all-gdb] Error 2 > > I am not sure why other people aren't running into this but I would like to > apply this patch to fix the build problem. Other people run into this too, Renlin Li opened PR 17718 for the build failure, https://sourceware.org/bugzilla/show_bug.cgi?id=17718 and Chen Gang proposed a similar fix to the same problem https://sourceware.org/ml/gdb-patches/2015-01/msg00011.html This build failure wasn't fixed because people discuss that we can replace system ("rm -rf") with ftw/rmdir/unlink, https://sourceware.org/ml/gdb-patches/2014-12/msg00501.html but the discussion looks stalled due to the holiday. It's unclear to me when the discussion can be closed and we have an acceptable fix. We should fix it by next week (7.9 branching). > > OK to checkin? I don't have an objection to this patch, but please wait for a while to see what do other people think of this patch.
> 2015-01-05 Steve Ellcey <sellcey@imgtec.com> > > * compile/compile.c (do_rmdir): Assign return value of system call. If we go through with a patch that eliminates the warning without actually doing anything about it, I'd request that a comment be added that explains why we're allowing ourselves to do so. In this case, as Yao explains, we're in the middle of a discussion about how to better write that code. > diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c > index 1d18bd7..f9f03f1 100644 > --- a/gdb/compile/compile.c > +++ b/gdb/compile/compile.c > @@ -169,10 +169,12 @@ do_rmdir (void *arg) > { > const char *dir = arg; > char *zap; > + int i; > > gdb_assert (strncmp (dir, TMP_PREFIX, strlen (TMP_PREFIX)) == 0); > zap = concat ("rm -rf ", dir, (char *) NULL); > - system (zap); > + /* GCC may generate warning if we ignore the return value of system call. */ > + i = system (zap); > } > > /* Return the name of the temporary directory to use for .o files, and Does it work to cast the result of the call to system to (void) instead? In your case, I fear that you'd be exchanging one warning (return value being ignored) by another (value assigned but never used).
On Tue, 2015-01-06 at 08:16 +0400, Joel Brobecker wrote: > > diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c > > index 1d18bd7..f9f03f1 100644 > > --- a/gdb/compile/compile.c > > +++ b/gdb/compile/compile.c > > @@ -169,10 +169,12 @@ do_rmdir (void *arg) > > { > > const char *dir = arg; > > char *zap; > > + int i; > > > > gdb_assert (strncmp (dir, TMP_PREFIX, strlen (TMP_PREFIX)) == 0); > > zap = concat ("rm -rf ", dir, (char *) NULL); > > - system (zap); > > + /* GCC may generate warning if we ignore the return value of system call. */ > > + i = system (zap); > > } > > > > /* Return the name of the temporary directory to use for .o files, and > > Does it work to cast the result of the call to system to (void) > instead? In your case, I fear that you'd be exchanging one warning > (return value being ignored) by another (value assigned but never > used). No, I tried using "(void) system (zap);" instead of "i = system (zap);" and I still got the warning message. I am going to respond on the "GDB 7.9 branching" email thread that I think this bug is a blocking bug since it breaks the build on some machines. I certainly think it needs to be addressed somehow before we release the next GDB. Steve Ellcey sellcey@imgtec.com
> > Does it work to cast the result of the call to system to (void) > > instead? In your case, I fear that you'd be exchanging one warning > > (return value being ignored) by another (value assigned but never > > used). > > No, I tried using "(void) system (zap);" instead of "i = system (zap);" > and I still got the warning message. In that case, I have no objection to your patch either, provided a small comment is added to explain why we allow ourselves to ignore the return value (and since you'll be touching that code anyways, I would also rename your variable to something more explicit, such as "ignored" or "unused" for instance). Thank you,
On Wed, 2015-01-07 at 08:13 +0400, Joel Brobecker wrote: > > > Does it work to cast the result of the call to system to (void) > > > instead? In your case, I fear that you'd be exchanging one warning > > > (return value being ignored) by another (value assigned but never > > > used). > > > > No, I tried using "(void) system (zap);" instead of "i = system (zap);" > > and I still got the warning message. > > In that case, I have no objection to your patch either, provided > a small comment is added to explain why we allow ourselves to ignore > the return value (and since you'll be touching that code anyways, > I would also rename your variable to something more explicit, such > as "ignored" or "unused" for instance). > > Thank you, I am not sure why we allow ourselves to ignore the return value. Maybe we shouldn't. Chen Gang submitted a different patch where the return value is checked. Should we use that instead? https://sourceware.org/ml/gdb-patches/2015-01/msg00011.html Steve Ellcey sellcey@imgtec.com
On 01/07/2015 06:36 PM, Steve Ellcey wrote: > I am not sure why we allow ourselves to ignore the return value. Maybe > we shouldn't. Chen Gang submitted a different patch where the return > value is checked. Should we use that instead? > > https://sourceware.org/ml/gdb-patches/2015-01/msg00011.html Yes, I think so. Jan's WIP patch to use ftw/fts instead of "rm -rf" also warns on fail. Meanwhile, I think Chen's patch is good. Thanks, Pedro Alves
On Wed, 7 Jan 2015, Steve Ellcey wrote: > > In that case, I have no objection to your patch either, provided > > a small comment is added to explain why we allow ourselves to ignore > > the return value (and since you'll be touching that code anyways, > > I would also rename your variable to something more explicit, such > > as "ignored" or "unused" for instance). > > > > Thank you, > > I am not sure why we allow ourselves to ignore the return value. Maybe > we shouldn't. Chen Gang submitted a different patch where the return > value is checked. Should we use that instead? > > https://sourceware.org/ml/gdb-patches/2015-01/msg00011.html The best idea IMHO as well. However I have concerns about this function overall in the first place. GDB supports hosts that have no `rm' program. It may support (although this I am less sure about) hosts that do not support the `system' C library call in a way we are used to; specifically there may not be a command processor available as noted in the ISO C document defining the API. Therefore I think it would be best to rewrite it to only use the relevant C library calls like `remove' and `rmdir' to recursively remove a directory; I wonder if actually we don't have something relevant already available in libiberty or gnulib. That of course does not mean we oughtn't to make a temporary fix to the immediate problem discussed here, I certainly don't object that. FWIW, Maciej
On 01/07/2015 07:29 PM, Maciej W. Rozycki wrote: > Therefore I think it would be best to rewrite it to only use the relevant > C library calls like `remove' and `rmdir' to recursively remove a > directory; I wonder if actually we don't have something relevant already > available in libiberty or gnulib. Jan's working on that already. See the ftw discussion, and the gnulib patches. > > That of course does not mean we oughtn't to make a temporary fix to the > immediate problem discussed here, I certainly don't object that. Yep. Thanks, Pedro Alves
On Wed, 7 Jan 2015, Pedro Alves wrote: > > Therefore I think it would be best to rewrite it to only use the relevant > > C library calls like `remove' and `rmdir' to recursively remove a > > directory; I wonder if actually we don't have something relevant already > > available in libiberty or gnulib. > > Jan's working on that already. See the ftw discussion, and the gnulib > patches. OK, great! Maciej
On Thu, 08 Jan 2015 00:33:06 +0100, Maciej W. Rozycki wrote: > On Wed, 7 Jan 2015, Pedro Alves wrote: > > > Therefore I think it would be best to rewrite it to only use the relevant > > > C library calls like `remove' and `rmdir' to recursively remove a > > > directory; I wonder if actually we don't have something relevant already > > > available in libiberty or gnulib. > > > > Jan's working on that already. See the ftw discussion, and the gnulib > > patches. > > OK, great! I have some patches here but I have suspended the work until the mingw pending patches get resolved so that one can test further patches on top of that: Re: [patch 1/2] mingw: update gnulib: prepare the sources https://sourceware.org/ml/gdb-patches/2014-12/msg00626.html Message-ID: <20141224222045.GA30482@host2.jankratochvil.net> + [patch 2/2] mingw: update gnulib: the gnulib files Message-ID: <20141222221330.GA31091@host2.jankratochvil.net> https://sourceware.org/ml/gdb-patches/2014-12/msg00610.html Jan
On Thu, 2015-01-08 at 22:12 +0100, Jan Kratochvil wrote: > On Thu, 08 Jan 2015 00:33:06 +0100, Maciej W. Rozycki wrote: > > On Wed, 7 Jan 2015, Pedro Alves wrote: > > > > Therefore I think it would be best to rewrite it to only use the relevant > > > > C library calls like `remove' and `rmdir' to recursively remove a > > > > directory; I wonder if actually we don't have something relevant already > > > > available in libiberty or gnulib. > > > > > > Jan's working on that already. See the ftw discussion, and the gnulib > > > patches. > > > > OK, great! > > I have some patches here but I have suspended the work until the mingw pending > patches get resolved so that one can test further patches on top of that: > Re: [patch 1/2] mingw: update gnulib: prepare the sources > https://sourceware.org/ml/gdb-patches/2014-12/msg00626.html > Message-ID: <20141224222045.GA30482@host2.jankratochvil.net> > + > [patch 2/2] mingw: update gnulib: the gnulib files > Message-ID: <20141222221330.GA31091@host2.jankratochvil.net> > https://sourceware.org/ml/gdb-patches/2014-12/msg00610.html > > > Jan Does that mean it won't get into GDB 7.9 (branching on Monday, January 12th). If so I would like to see Chen's patch get checked in as a temporary build fix before the branch. I haven't seen any objection to Chen's patch but I haven't seen an official approval either. Steve Ellcey sellcey@imgtec.com
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
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? Steve Ellcey sellcey@imgtec.com
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. Thanks.
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c index 1d18bd7..f9f03f1 100644 --- a/gdb/compile/compile.c +++ b/gdb/compile/compile.c @@ -169,10 +169,12 @@ do_rmdir (void *arg) { const char *dir = arg; char *zap; + int i; gdb_assert (strncmp (dir, TMP_PREFIX, strlen (TMP_PREFIX)) == 0); zap = concat ("rm -rf ", dir, (char *) NULL); - system (zap); + /* GCC may generate warning if we ignore the return value of system call. */ + i = system (zap); } /* Return the name of the temporary directory to use for .o files, and