Fix build problem with system call in compile/compile.c

  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.
  

"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.