Patchwork Correct use of ! and &

login
register
mail settings
Submitter Blue Swirl
Date Aug. 21, 2010, 9:42 a.m.
Message ID <AANLkTi=g3NHwzT6JuU30V0iBB906PHM5du7BsQY4qfT=@mail.gmail.com>
Download mbox | patch
Permalink /patch/62344/
State New
Headers show

Comments

Blue Swirl - Aug. 21, 2010, 9:42 a.m.
Combining bitwise AND and logical NOT is suspicious.

Fixed by this Coccinelle script:
// From http://article.gmane.org/gmane.linux.kernel/646367
@@ expression E1,E2; @@
(
  !E1 & !E2
|
- !E1 & E2
+ !(E1 & E2)
)

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---

Maybe the middle hunk should be fixed this way instead:
-            } else if ((rw == 1) & !matching->d) {
+            } else if ((rw == 1) && !matching->d) {

---
 hw/etraxfs_eth.c    |    2 +-
 target-sh4/helper.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

                 *prot = PAGE_READ;
@@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
target_ulong * physical,
     }

     /* If MMU is disabled, return the corresponding physical page */
-    if (!env->mmucr & MMUCR_AT) {
+    if (!(env->mmucr & MMUCR_AT)) {
 	*physical = address & 0x1FFFFFFF;
 	*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
 	return MMU_OK;
Markus Armbruster - Aug. 21, 2010, 1:19 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> Combining bitwise AND and logical NOT is suspicious.
>
> Fixed by this Coccinelle script:
> // From http://article.gmane.org/gmane.linux.kernel/646367
> @@ expression E1,E2; @@
> (
>   !E1 & !E2
> |
> - !E1 & E2
> + !(E1 & E2)
> )
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>
> Maybe the middle hunk should be fixed this way instead:
> -            } else if ((rw == 1) & !matching->d) {
> +            } else if ((rw == 1) && !matching->d) {
>
> ---
>  hw/etraxfs_eth.c    |    2 +-
>  target-sh4/helper.c |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
> index b897c9c..ade96f1 100644
> --- a/hw/etraxfs_eth.c
> +++ b/hw/etraxfs_eth.c
> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
> const unsigned char *sa)
>
>  	/* First bit on the wire of a MAC address signals multicast or
>  	   physical address.  */
> -	if (!m_individual && !sa[0] & 1)
> +	if (!m_individual && !(sa[0] & 1))

Doesn't this fix a bug?  If yes, then the commit message should state
that, and assess impact.

>  		return 0;
>
>  	/* Calculate the hash index for the GA registers. */
> diff --git a/target-sh4/helper.c b/target-sh4/helper.c
> index 9e70352..e457904 100644
> --- a/target-sh4/helper.c
> +++ b/target-sh4/helper.c
> @@ -357,7 +357,7 @@ static int get_mmu_address(CPUState * env,
> target_ulong * physical,
>                      MMU_DTLB_VIOLATION_READ;
>              } else if ((rw == 1) && !(matching->pr & 1)) {
>                  n = MMU_DTLB_VIOLATION_WRITE;
> -            } else if ((rw == 1) & !matching->d) {
> +            } else if (!(matching->d & (rw == 1))) {
>                  n = MMU_DTLB_INITIAL_WRITE;
>              } else {
>                  *prot = PAGE_READ;

Way too clever for my taste.  I'd prefer the alternative fix you
mentioned above.

> @@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
> target_ulong * physical,
>      }
>
>      /* If MMU is disabled, return the corresponding physical page */
> -    if (!env->mmucr & MMUCR_AT) {
> +    if (!(env->mmucr & MMUCR_AT)) {
>  	*physical = address & 0x1FFFFFFF;
>  	*prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>  	return MMU_OK;


Doesn't this fix a bug?
Blue Swirl - Aug. 21, 2010, 2:49 p.m.
On Sat, Aug 21, 2010 at 1:19 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> Combining bitwise AND and logical NOT is suspicious.
>>
>> Fixed by this Coccinelle script:
>> // From http://article.gmane.org/gmane.linux.kernel/646367
>> @@ expression E1,E2; @@
>> (
>>   !E1 & !E2
>> |
>> - !E1 & E2
>> + !(E1 & E2)
>> )
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>
>> Maybe the middle hunk should be fixed this way instead:
>> -            } else if ((rw == 1) & !matching->d) {
>> +            } else if ((rw == 1) && !matching->d) {
>>
>> ---
>>  hw/etraxfs_eth.c    |    2 +-
>>  target-sh4/helper.c |    4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
>> index b897c9c..ade96f1 100644
>> --- a/hw/etraxfs_eth.c
>> +++ b/hw/etraxfs_eth.c
>> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
>> const unsigned char *sa)
>>
>>       /* First bit on the wire of a MAC address signals multicast or
>>          physical address.  */
>> -     if (!m_individual && !sa[0] & 1)
>> +     if (!m_individual && !(sa[0] & 1))
>
> Doesn't this fix a bug?  If yes, then the commit message should state
> that, and assess impact.

I don't know, the patch wasn't a result from any specific bug hunting
but from syntax analysis.

>>               return 0;
>>
>>       /* Calculate the hash index for the GA registers. */
>> diff --git a/target-sh4/helper.c b/target-sh4/helper.c
>> index 9e70352..e457904 100644
>> --- a/target-sh4/helper.c
>> +++ b/target-sh4/helper.c
>> @@ -357,7 +357,7 @@ static int get_mmu_address(CPUState * env,
>> target_ulong * physical,
>>                      MMU_DTLB_VIOLATION_READ;
>>              } else if ((rw == 1) && !(matching->pr & 1)) {
>>                  n = MMU_DTLB_VIOLATION_WRITE;
>> -            } else if ((rw == 1) & !matching->d) {
>> +            } else if (!(matching->d & (rw == 1))) {
>>                  n = MMU_DTLB_INITIAL_WRITE;
>>              } else {
>>                  *prot = PAGE_READ;
>
> Way too clever for my taste.  I'd prefer the alternative fix you
> mentioned above.

The effect is different, though.

>> @@ -407,7 +407,7 @@ static int get_physical_address(CPUState * env,
>> target_ulong * physical,
>>      }
>>
>>      /* If MMU is disabled, return the corresponding physical page */
>> -    if (!env->mmucr & MMUCR_AT) {
>> +    if (!(env->mmucr & MMUCR_AT)) {
>>       *physical = address & 0x1FFFFFFF;
>>       *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>>       return MMU_OK;
>
>
> Doesn't this fix a bug?

Probably.
Edgar Iglesias - Aug. 26, 2010, 1:23 p.m.
On Sat, Aug 21, 2010 at 09:42:51AM +0000, Blue Swirl wrote:
> Combining bitwise AND and logical NOT is suspicious.
> 
> Fixed by this Coccinelle script:
> // From http://article.gmane.org/gmane.linux.kernel/646367
> @@ expression E1,E2; @@
> (
>   !E1 & !E2
> |
> - !E1 & E2
> + !(E1 & E2)
> )
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
> 
> Maybe the middle hunk should be fixed this way instead:
> -            } else if ((rw == 1) & !matching->d) {
> +            } else if ((rw == 1) && !matching->d) {
> 
> ---
>  hw/etraxfs_eth.c    |    2 +-
>  target-sh4/helper.c |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
> index b897c9c..ade96f1 100644
> --- a/hw/etraxfs_eth.c
> +++ b/hw/etraxfs_eth.c
> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
> const unsigned char *sa)
> 
>  	/* First bit on the wire of a MAC address signals multicast or
>  	   physical address.  */
> -	if (!m_individual && !sa[0] & 1)
> +	if (!m_individual && !(sa[0] & 1))
>  		return 0;


Yep, the etrax part is a bug and your patch looks OK to me.

Thanks

Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Blue Swirl - Aug. 26, 2010, 6:05 p.m.
On Thu, Aug 26, 2010 at 1:23 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Sat, Aug 21, 2010 at 09:42:51AM +0000, Blue Swirl wrote:
>> Combining bitwise AND and logical NOT is suspicious.
>>
>> Fixed by this Coccinelle script:
>> // From http://article.gmane.org/gmane.linux.kernel/646367
>> @@ expression E1,E2; @@
>> (
>>   !E1 & !E2
>> |
>> - !E1 & E2
>> + !(E1 & E2)
>> )
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>
>> Maybe the middle hunk should be fixed this way instead:
>> -            } else if ((rw == 1) & !matching->d) {
>> +            } else if ((rw == 1) && !matching->d) {
>>
>> ---
>>  hw/etraxfs_eth.c    |    2 +-
>>  target-sh4/helper.c |    4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
>> index b897c9c..ade96f1 100644
>> --- a/hw/etraxfs_eth.c
>> +++ b/hw/etraxfs_eth.c
>> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
>> const unsigned char *sa)
>>
>>       /* First bit on the wire of a MAC address signals multicast or
>>          physical address.  */
>> -     if (!m_individual && !sa[0] & 1)
>> +     if (!m_individual && !(sa[0] & 1))
>>               return 0;
>
>
> Yep, the etrax part is a bug and your patch looks OK to me.
>
> Thanks
>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Thanks for the review, I applied this part of the patch.
Markus Armbruster - Aug. 27, 2010, 9:03 a.m.
"Edgar E. Iglesias" <edgar.iglesias@gmail.com> writes:

> On Sat, Aug 21, 2010 at 09:42:51AM +0000, Blue Swirl wrote:
>> Combining bitwise AND and logical NOT is suspicious.
>> 
>> Fixed by this Coccinelle script:
>> // From http://article.gmane.org/gmane.linux.kernel/646367
>> @@ expression E1,E2; @@
>> (
>>   !E1 & !E2
>> |
>> - !E1 & E2
>> + !(E1 & E2)
>> )
>> 
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>> 
>> Maybe the middle hunk should be fixed this way instead:
>> -            } else if ((rw == 1) & !matching->d) {
>> +            } else if ((rw == 1) && !matching->d) {
>> 
>> ---
>>  hw/etraxfs_eth.c    |    2 +-
>>  target-sh4/helper.c |    4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
>> index b897c9c..ade96f1 100644
>> --- a/hw/etraxfs_eth.c
>> +++ b/hw/etraxfs_eth.c
>> @@ -464,7 +464,7 @@ static int eth_match_groupaddr(struct fs_eth *eth,
>> const unsigned char *sa)
>> 
>>  	/* First bit on the wire of a MAC address signals multicast or
>>  	   physical address.  */
>> -	if (!m_individual && !sa[0] & 1)
>> +	if (!m_individual && !(sa[0] & 1))
>>  		return 0;
>
>
> Yep, the etrax part is a bug and your patch looks OK to me.
>
> Thanks
>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Such review is exactly what we need when static analysis spots fishy
code.  Thanks!

Volunteers to look at the other two hunks?

Patch

diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index b897c9c..ade96f1 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -464,7 +464,7 @@  static int eth_match_groupaddr(struct fs_eth *eth,
const unsigned char *sa)

 	/* First bit on the wire of a MAC address signals multicast or
 	   physical address.  */
-	if (!m_individual && !sa[0] & 1)
+	if (!m_individual && !(sa[0] & 1))
 		return 0;

 	/* Calculate the hash index for the GA registers. */
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 9e70352..e457904 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -357,7 +357,7 @@  static int get_mmu_address(CPUState * env,
target_ulong * physical,
                     MMU_DTLB_VIOLATION_READ;
             } else if ((rw == 1) && !(matching->pr & 1)) {
                 n = MMU_DTLB_VIOLATION_WRITE;
-            } else if ((rw == 1) & !matching->d) {
+            } else if (!(matching->d & (rw == 1))) {
                 n = MMU_DTLB_INITIAL_WRITE;
             } else {