[PATCH] Empty if body in linux-record.c

[PATCH] Empty if body in linux-record.c

[Holger Hans Peter Freyther]

Hi all,

this is one the issues reported by "clang --analyze" and it appears to
me that the code was broken in the same way since the initial check-in
of these routines, looking at the tests in gdb.record it does not appear
that these are covered at all.

I'm pretty sure that the code as is is wrong and that my patch is sensitive
but I'm not sure how to properly proof this. Currently the only way would
be to run a record an app that is using the syscalls and check if recording
fails/reports an issue and stops reporting it after wards?

I'm not really known to the patch contribution of gdb so I'm not sure if I'm
following the proper process here...anyway here is a patch with a
ChangeLog entry.


From 7e53969f18043d7cd997531934e850ccb6b95190 Mon Sep 17 00:00:00 2001
From: Holger Hans Peter Freyther <zecke@selfish.org>
Date: Sun, 14 Mar 2010 05:31:40 +0100
Subject: [PATCH] 2010-03-14  Holger Hans Peter Freyther  <zecke@selfish.org>

	* linux-record.c (record_linux_msghdr): Fix a compiler warning
	about an empty if body that has existed since the addition of the
	code. This change will change the return value of the function from
	always returning -1 to returning -1 only if calling record_arch_list_add_mem
	is failing. The inner for loop will also be iterated more than once
	now.
---
 gdb/ChangeLog      |    9 +++++++++
 gdb/linux-record.c |    4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d9e6fd8..9f6528d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2010-03-14  Holger Hans Peter Freyther  <zecke@selfish.org>
+
+	* linux-record.c (record_linux_msghdr): Fix a compiler warning
+	about an empty if body that has existed since the addition of the
+	code. This change will change the return value of the function from
+	always returning -1 to returning -1 only if calling record_arch_list_add_mem
+	is failing. The inner for loop will also be iterated more than once
+	now.
+
 2010-03-12  Tom Tromey  <tromey@redhat.com>
 
 	PR c++/9708:
diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index 1c6aa9f..74ef9bd 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -192,7 +192,7 @@ record_linux_msghdr (struct regcache *regcache,
           tmpint = (int) extract_unsigned_integer (iov + tdep->size_pointer,
                                                    tdep->size_size_t,
                                                    byte_order);
-          if (record_arch_list_add_mem (tmpaddr, tmpint));
+          if (record_arch_list_add_mem (tmpaddr, tmpint))
             return -1;
           addr += tdep->size_iovec;
         }
@@ -203,7 +203,7 @@ record_linux_msghdr (struct regcache *regcache,
   addr = extract_unsigned_integer (a, tdep->size_pointer, byte_order);
   a += tdep->size_pointer;
   tmpint = (int) extract_unsigned_integer (a, tdep->size_size_t, byte_order);
-  if (record_arch_list_add_mem ((CORE_ADDR) addr, tmpint));
+  if (record_arch_list_add_mem ((CORE_ADDR) addr, tmpint))
     return -1;
 
   return 0;
-- 
1.7.0

Re: [PATCH] Empty if body in linux-record.c

[Joel Brobecker]

> this is one the issues reported by "clang --analyze" and it appears to
> me that the code was broken in the same way since the initial check-in
> of these routines, looking at the tests in gdb.record it does not appear
> that these are covered at all.

I think you are right. Hui should be able to confirm that the code
was indeed broken and that the trailing semicolon was not intended.

> I'm not really known to the patch contribution of gdb so I'm not sure if I'm
> following the proper process here...anyway here is a patch with a
> ChangeLog entry.

Pretty good for a first try :). Just a couple of comments:

  1. You should indicate how you tested this. Normally, you run
     the testsuite before and after, and compare the associated gdb.sum
     file, to make sure that you did not introduce any regression.

     In this particular case, you also need to make sure that process
     record testing is activated, which it is not by default.
     Please have a look at
     http://sourceware.org/gdb/wiki/ProcessRecord#Testing.

   2. Your ChangeLog entry is much too verbose:

> 	* linux-record.c (record_linux_msghdr): Fix a compiler warning
> 	about an empty if body that has existed since the addition of the
> 	code. This change will change the return value of the function from
> 	always returning -1 to returning -1 only if calling record_arch_list_add_mem
> 	is failing. The inner for loop will also be iterated more than once
> 	now.

      You just need to say *what* you've done, not why or how.
      This is explained in more details in the Gnu Coding Standards
      manual. (I sometimes indulge myself, and add an explaination,
      but it's because it's a very very short one)

      On possibility would be:

      * linux-record.c (record_linux_msghdr): Remove unintended semicolons.

Would you like write access to the GDB repository?

-- 
Joel

Re: [PATCH] Empty if body in linux-record.c

[Hui Zhu]

That is great!
Thanks Holger.

Hi Joel,

Do you think I can help this guy check-in this patch directly?

Thanks,
Hui

On Mon, Mar 15, 2010 at 09:00, Joel Brobecker <brobecker@adacore.com> wrote:
>> this is one the issues reported by "clang --analyze" and it appears to
>> me that the code was broken in the same way since the initial check-in
>> of these routines, looking at the tests in gdb.record it does not appear
>> that these are covered at all.
>
> I think you are right. Hui should be able to confirm that the code
> was indeed broken and that the trailing semicolon was not intended.
>
>> I'm not really known to the patch contribution of gdb so I'm not sure if I'm
>> following the proper process here...anyway here is a patch with a
>> ChangeLog entry.
>
> Pretty good for a first try :). Just a couple of comments:
>
>  1. You should indicate how you tested this. Normally, you run
>     the testsuite before and after, and compare the associated gdb.sum
>     file, to make sure that you did not introduce any regression.
>
>     In this particular case, you also need to make sure that process
>     record testing is activated, which it is not by default.
>     Please have a look at
>     http://sourceware.org/gdb/wiki/ProcessRecord#Testing.
>
>   2. Your ChangeLog entry is much too verbose:
>
>>       * linux-record.c (record_linux_msghdr): Fix a compiler warning
>>       about an empty if body that has existed since the addition of the
>>       code. This change will change the return value of the function from
>>       always returning -1 to returning -1 only if calling record_arch_list_add_mem
>>       is failing. The inner for loop will also be iterated more than once
>>       now.
>
>      You just need to say *what* you've done, not why or how.
>      This is explained in more details in the Gnu Coding Standards
>      manual. (I sometimes indulge myself, and add an explaination,
>      but it's because it's a very very short one)
>
>      On possibility would be:
>
>      * linux-record.c (record_linux_msghdr): Remove unintended semicolons.
>
> Would you like write access to the GDB repository?
>
> --
> Joel
>

Re: [PATCH] Empty if body in linux-record.c

[Joel Brobecker]

> Do you think I can help this guy check-in this patch directly?

Sure, go ahead for this one.

(for the record, although this is definitly a tiny and obvious patch,
Holger has an FSF assignment on file for GDB)

-- 
Joel

Re: [PATCH] Empty if body in linux-record.c

[Joel Brobecker]

> > Do you think I can help this guy check-in this patch directly?
> 
> Sure, go ahead for this one.

BTW: Hui, can you put the patch in the gdb-7.1 branch as well?
I think it's sufficiently obvious that it can only be beneficial.

Thanks,
-- 
Joel

Re: [PATCH] Empty if body in linux-record.c

[Hui Zhu]

7.1 branch and head checked in.

Thanks,
Hui

On Tue, Mar 16, 2010 at 23:59, Joel Brobecker <brobecker@adacore.com> wrote:
>> > Do you think I can help this guy check-in this patch directly?
>>
>> Sure, go ahead for this one.
>
> BTW: Hui, can you put the patch in the gdb-7.1 branch as well?
> I think it's sufficiently obvious that it can only be beneficial.
>
> Thanks,
> --
> Joel
>

Re: [PATCH] Empty if body in linux-record.c

[Holger Hans Peter Freyther]

On Tuesday 16 March 2010 18:07:56 Hui Zhu wrote:
> 7.1 branch and head checked in.

thank you.