Re: [Qemu-devel] [PATCH] Correct use of ! and &

[Markus Armbruster]

Blue Swirl <address@hidden> 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 <address@hidden>
> ---
>
> 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?