Patchwork sparc-dis: Remove redundant NULL check

login
register
mail settings
Submitter Stefan Weil
Date Sept. 3, 2012, 8:23 p.m.
Message ID <1346703803-22003-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/181410/
State Under Review
Headers show

Comments

Stefan Weil - Sept. 3, 2012, 8:23 p.m.
Report from smatch:
sparc-dis.c:2664 build_hash_table(14) info:
 redundant null check on hash_buf calling free()

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Coding style was not fixed.

- sw

 sparc-dis.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Stefan Hajnoczi - Sept. 5, 2012, 9:34 a.m.
On Mon, Sep 03, 2012 at 10:23:23PM +0200, Stefan Weil wrote:
> Report from smatch:
> sparc-dis.c:2664 build_hash_table(14) info:
>  redundant null check on hash_buf calling free()
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> Coding style was not fixed.
> 
> - sw
> 
>  sparc-dis.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Thanks, applied to the trivial patches tree:
https://github.com/stefanha/qemu/commits/trivial-patches

Stefan
Andreas Färber - Sept. 5, 2012, 3:51 p.m.
Am 03.09.2012 22:23, schrieb Stefan Weil:
> Report from smatch:
> sparc-dis.c:2664 build_hash_table(14) info:
>  redundant null check on hash_buf calling free()
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> Coding style was not fixed.
> 
> - sw
> 
>  sparc-dis.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sparc-dis.c b/sparc-dis.c
> index cdd337a..ef28835 100644
> --- a/sparc-dis.c
> +++ b/sparc-dis.c
> @@ -2660,8 +2660,7 @@ build_hash_table (const sparc_opcode **opcode_table,
>  
>    memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
>    memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
> -  if (hash_buf != NULL)
> -    free (hash_buf);
> +  free (hash_buf);
>    hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
>    for (i = num_opcodes - 1; i >= 0; --i)
>      {

*-dis sounds like binutils - did upstream drop the if, too?
If not, then diverging for a non-issue does not seem necessary.

Having said that, it's amazing what fixes you come up with. Did you
describe the use of that tool in some posting or in the Wiki?

Regards,
Andreas
Stefan Hajnoczi - Sept. 5, 2012, 4:55 p.m.
On Wed, Sep 5, 2012 at 4:51 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 03.09.2012 22:23, schrieb Stefan Weil:
>> Report from smatch:
>> sparc-dis.c:2664 build_hash_table(14) info:
>>  redundant null check on hash_buf calling free()
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> Coding style was not fixed.
>>
>> - sw
>>
>>  sparc-dis.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/sparc-dis.c b/sparc-dis.c
>> index cdd337a..ef28835 100644
>> --- a/sparc-dis.c
>> +++ b/sparc-dis.c
>> @@ -2660,8 +2660,7 @@ build_hash_table (const sparc_opcode **opcode_table,
>>
>>    memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
>>    memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
>> -  if (hash_buf != NULL)
>> -    free (hash_buf);
>> +  free (hash_buf);
>>    hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
>>    for (i = num_opcodes - 1; i >= 0; --i)
>>      {
>
> *-dis sounds like binutils - did upstream drop the if, too?
> If not, then diverging for a non-issue does not seem necessary.

Ah, good point.  I don't think we should apply this patch :P.

Dropping the patch from qemu-trivial until this discussion finishes.

Stefan
Stefan Weil - Sept. 5, 2012, 5:15 p.m.
Am 05.09.2012 18:55, schrieb Stefan Hajnoczi:
> On Wed, Sep 5, 2012 at 4:51 PM, Andreas Färber<afaerber@suse.de>  wrote:
>> Am 03.09.2012 22:23, schrieb Stefan Weil:
>>> Report from smatch:
>>> sparc-dis.c:2664 build_hash_table(14) info:
>>>   redundant null check on hash_buf calling free()
>>>
>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>>> ---
>>>
>>> Coding style was not fixed.
>>>
>>> - sw
>>>
>>>   sparc-dis.c |    3 +--
>>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/sparc-dis.c b/sparc-dis.c
>>> index cdd337a..ef28835 100644
>>> --- a/sparc-dis.c
>>> +++ b/sparc-dis.c
>>> @@ -2660,8 +2660,7 @@ build_hash_table (const sparc_opcode **opcode_table,
>>>
>>>     memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
>>>     memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
>>> -  if (hash_buf != NULL)
>>> -    free (hash_buf);
>>> +  free (hash_buf);
>>>     hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
>>>     for (i = num_opcodes - 1; i>= 0; --i)
>>>       {
>>
>> *-dis sounds like binutils - did upstream drop the if, too?
>> If not, then diverging for a non-issue does not seem necessary.
>
> Ah, good point.  I don't think we should apply this patch :P.
>
> Dropping the patch from qemu-trivial until this discussion finishes.
>
> Stefan

AFAIK, binutils moved to GPL 3. Therefore I don't expect that
QEMU will update to upstream in the next years.

We'll have to maintain the code which we have.

Try "git log *-dis.c" or "gitk *-dis.c": there are already lots
of more trivial changes which got applied to the disassembler files.

=> The patch should be applied.

Regards,
Stefan
Stefan Weil - Sept. 5, 2012, 5:45 p.m.
Am 05.09.2012 19:15, schrieb Stefan Weil:
>
> AFAIK, binutils moved to GPL 3. Therefore I don't expect that
> QEMU will update to upstream in the next years.
>
> We'll have to maintain the code which we have.
>
> Try "git log *-dis.c" or "gitk *-dis.c": there are already lots
> of more trivial changes which got applied to the disassembler files.
>
> => The patch should be applied.
>
> Regards,
> Stefan

Here is some additional information:

binutils switched from GPL 2 to GPL 3 with version 2.18:

Changes in 2.18:

* The binutils sources are now released under version 3 of the GNU General
   Public License.


sparc-dis.c is already based on 2.17, so we won't get anything newer.
Even the latest version still uses the redundant NULL check, so I
can send my patch to the binutils maintainers, too.

Regards,
Stefan
Stefan Weil - Sept. 5, 2012, 6:13 p.m.
Am 05.09.2012 17:51, schrieb Andreas Färber:
> Am 03.09.2012 22:23, schrieb Stefan Weil:
>    
>> Report from smatch:
>> sparc-dis.c:2664 build_hash_table(14) info:
>>   redundant null check on hash_buf calling free()
>>
>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>> ---
>>
>> Coding style was not fixed.
>>
>> - sw
>>
>>   sparc-dis.c |    3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/sparc-dis.c b/sparc-dis.c
>> index cdd337a..ef28835 100644
>> --- a/sparc-dis.c
>> +++ b/sparc-dis.c
>> @@ -2660,8 +2660,7 @@ build_hash_table (const sparc_opcode **opcode_table,
>>
>>     memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
>>     memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
>> -  if (hash_buf != NULL)
>> -    free (hash_buf);
>> +  free (hash_buf);
>>     hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
>>     for (i = num_opcodes - 1; i>= 0; --i)
>>       {
>>      
> *-dis sounds like binutils - did upstream drop the if, too?
> If not, then diverging for a non-issue does not seem necessary.
>
> Having said that, it's amazing what fixes you come up with. Did you
> describe the use of that tool in some posting or in the Wiki?
>    

I just added some initial text to http://wiki.qemu.org/Testing.

> Regards,
> Andreas
>
>
Stefan Weil - Sept. 20, 2012, 5:03 p.m.
Am 05.09.2012 19:45, schrieb Stefan Weil:
> Am 05.09.2012 19:15, schrieb Stefan Weil:
>>
>> AFAIK, binutils moved to GPL 3. Therefore I don't expect that
>> QEMU will update to upstream in the next years.
>>
>> We'll have to maintain the code which we have.
>>
>> Try "git log *-dis.c" or "gitk *-dis.c": there are already lots
>> of more trivial changes which got applied to the disassembler files.
>>
>> => The patch should be applied.
>>
>> Regards,
>> Stefan
>
> Here is some additional information:
>
> binutils switched from GPL 2 to GPL 3 with version 2.18:
>
> Changes in 2.18:
>
> * The binutils sources are now released under version 3 of the GNU 
> General
>   Public License.
>
>
> sparc-dis.c is already based on 2.17, so we won't get anything newer.
> Even the latest version still uses the redundant NULL check, so I
> can send my patch to the binutils maintainers, too.
>
> Regards,
> Stefan
>


Ping? If nobody objects, I suggest to apply the patch via qemu-trivial.

Regards

Stefan W.
Andreas Färber - Sept. 21, 2012, 8:06 a.m.
Am 20.09.2012 19:03, schrieb Stefan Weil:
> Am 05.09.2012 19:45, schrieb Stefan Weil:
>> Am 05.09.2012 19:15, schrieb Stefan Weil:
>>>
>>> AFAIK, binutils moved to GPL 3. Therefore I don't expect that
>>> QEMU will update to upstream in the next years.
>>>
>>> We'll have to maintain the code which we have.
>>>
>>> Try "git log *-dis.c" or "gitk *-dis.c": there are already lots
>>> of more trivial changes which got applied to the disassembler files.
>>>
>>> => The patch should be applied.
>>>
>>> Regards,
>>> Stefan
>>
>> Here is some additional information:
>>
>> binutils switched from GPL 2 to GPL 3 with version 2.18:
>>
>> Changes in 2.18:
>>
>> * The binutils sources are now released under version 3 of the GNU
>> General
>>   Public License.
>>
>>
>> sparc-dis.c is already based on 2.17, so we won't get anything newer.
>> Even the latest version still uses the redundant NULL check, so I
>> can send my patch to the binutils maintainers, too.

So did you? And did they accept it?

Regards,
Andreas

> Ping? If nobody objects, I suggest to apply the patch via qemu-trivial.
> 
> Regards
> 
> Stefan W.

Patch

diff --git a/sparc-dis.c b/sparc-dis.c
index cdd337a..ef28835 100644
--- a/sparc-dis.c
+++ b/sparc-dis.c
@@ -2660,8 +2660,7 @@  build_hash_table (const sparc_opcode **opcode_table,
 
   memset (hash_table, 0, HASH_SIZE * sizeof (hash_table[0]));
   memset (hash_count, 0, HASH_SIZE * sizeof (hash_count[0]));
-  if (hash_buf != NULL)
-    free (hash_buf);
+  free (hash_buf);
   hash_buf = malloc (sizeof (* hash_buf) * num_opcodes);
   for (i = num_opcodes - 1; i >= 0; --i)
     {