[Patch] Fix build problem with system call in compile/compile.c

[Patch] Fix build problem with system call in compile/compile.c

[Steve Ellcey]

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.


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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Yao Qi]

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

-- 
Yao (齐尧)

Re: [Patch] Fix build problem with system call in compile/compile.c

[Joel Brobecker]

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

-- 
Joel

Re: [Patch] Fix build problem with system call in compile/compile.c

[Steve Ellcey]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Joel Brobecker]

> > 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,
-- 
Joel

Re: [Patch] Fix build problem with system call in compile/compile.c

[Steve Ellcey]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Pedro Alves]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Maciej W. Rozycki]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Pedro Alves]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Maciej W. Rozycki]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Jan Kratochvil]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Steve Ellcey]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Pedro Alves]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Steve Ellcey]

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

Re: [Patch] Fix build problem with system call in compile/compile.c

[Chen Gang S]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Patch] Fix build problem with system call in compile/compile.c

[Pedro Alves]

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(-)

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"
 
 \f
 
@@ -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
-- 
1.9.3

Re: [Patch] Fix build problem with system call in compile/compile.c

[Chen Gang S]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Patch] Fix build problem with system call in compile/compile.c

[Chen Gang S]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Patch] Fix build problem with system call in compile/compile.c

[Chen Gang S]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Patch] Fix build problem with system call in compile/compile.c

[Joel Brobecker]

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

-- 
Joel