Patchwork Correct use of ! and &

login
register
mail settings
Submitter Blue Swirl
Date Jan. 14, 2011, 10:05 p.m.
Message ID <AANLkTinf=y3AqxKPG6gLriRMbsXBaS=Uds37qrbx4grB@mail.gmail.com>
Download mbox | patch
Permalink /patch/79016/
State New
Headers show

Comments

Blue Swirl - Jan. 14, 2011, 10:05 p.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>
---
 target-sh4/helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

                 *prot = PAGE_READ;
@@ -430,7 +430,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;
Aurelien Jarno - Jan. 14, 2011, 10:25 p.m.
On Fri, Jan 14, 2011 at 10:05:11PM +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>
> ---
>  target-sh4/helper.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-sh4/helper.c b/target-sh4/helper.c
> index 2343366..c6af959 100644
> --- a/target-sh4/helper.c
> +++ b/target-sh4/helper.c
> @@ -380,7 +380,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))) {

Here, we really want to do a logical comparison, so && is probably
better. Note however that matching->d is a single bit so the code is
doing what it is supposed to do.

>                  n = MMU_DTLB_INITIAL_WRITE;
>              } else {
>                  *prot = PAGE_READ;
> @@ -430,7 +430,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;

This one is correct.

Thanks for spotting these issues.
Aurelien Jarno - Jan. 15, 2011, 12:59 p.m.
On Fri, Jan 14, 2011 at 11:25:51PM +0100, Aurelien Jarno wrote:
> On Fri, Jan 14, 2011 at 10:05:11PM +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>
> > ---
> >  target-sh4/helper.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-sh4/helper.c b/target-sh4/helper.c
> > index 2343366..c6af959 100644
> > --- a/target-sh4/helper.c
> > +++ b/target-sh4/helper.c
> > @@ -380,7 +380,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))) {
> 
> Here, we really want to do a logical comparison, so && is probably
> better. Note however that matching->d is a single bit so the code is
> doing what it is supposed to do.
> 
> >                  n = MMU_DTLB_INITIAL_WRITE;
> >              } else {
> >                  *prot = PAGE_READ;
> > @@ -430,7 +430,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;
> 
> This one is correct.
> 
> Thanks for spotting these issues.
> 

I have just committed a patch to fix them.

Patch

diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 2343366..c6af959 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -380,7 +380,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 {