* [Qemu-devel] [PATCH] Correct use of ! and &
@ 2010-08-21 9:42 Blue Swirl
2010-08-21 13:19 ` Markus Armbruster
2010-08-26 13:23 ` Edgar E. Iglesias
0 siblings, 2 replies; 7+ messages in thread
From: Blue Swirl @ 2010-08-21 9:42 UTC (permalink / raw)
To: qemu-devel
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;
/* 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;
@@ -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;
--
1.6.2.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] Correct use of ! and &
2010-08-21 9:42 [Qemu-devel] [PATCH] Correct use of ! and & Blue Swirl
@ 2010-08-21 13:19 ` Markus Armbruster
2010-08-21 14:49 ` Blue Swirl
2010-08-26 13:23 ` Edgar E. Iglesias
1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2010-08-21 13:19 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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?
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] Correct use of ! and &
2010-08-21 13:19 ` Markus Armbruster
@ 2010-08-21 14:49 ` Blue Swirl
0 siblings, 0 replies; 7+ messages in thread
From: Blue Swirl @ 2010-08-21 14:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] Correct use of ! and &
2010-08-21 9:42 [Qemu-devel] [PATCH] Correct use of ! and & Blue Swirl
2010-08-21 13:19 ` Markus Armbruster
@ 2010-08-26 13:23 ` Edgar E. Iglesias
2010-08-26 18:05 ` Blue Swirl
2010-08-27 9:03 ` Markus Armbruster
1 sibling, 2 replies; 7+ messages in thread
From: Edgar E. Iglesias @ 2010-08-26 13:23 UTC (permalink / raw)
To: Blue Swirl; +Cc: qemu-devel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] Correct use of ! and &
2010-08-26 13:23 ` Edgar E. Iglesias
@ 2010-08-26 18:05 ` Blue Swirl
2010-08-27 9:03 ` Markus Armbruster
1 sibling, 0 replies; 7+ messages in thread
From: Blue Swirl @ 2010-08-26 18:05 UTC (permalink / raw)
To: Edgar E. Iglesias; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] Correct use of ! and &
2010-08-26 13:23 ` Edgar E. Iglesias
2010-08-26 18:05 ` Blue Swirl
@ 2010-08-27 9:03 ` Markus Armbruster
1 sibling, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2010-08-27 9:03 UTC (permalink / raw)
To: Edgar E. Iglesias; +Cc: Blue Swirl, qemu-devel
"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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] Correct use of ! and &
@ 2011-01-14 22:05 Blue Swirl
0 siblings, 0 replies; 7+ messages in thread
From: Blue Swirl @ 2011-01-14 22:05 UTC (permalink / raw)
To: Aurelien Jarno, qemu-devel
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))) {
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;
--
1.6.2.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-01-14 22:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-21 9:42 [Qemu-devel] [PATCH] Correct use of ! and & Blue Swirl
2010-08-21 13:19 ` Markus Armbruster
2010-08-21 14:49 ` Blue Swirl
2010-08-26 13:23 ` Edgar E. Iglesias
2010-08-26 18:05 ` Blue Swirl
2010-08-27 9:03 ` Markus Armbruster
-- strict thread matches above, loose matches on Subject: below --
2011-01-14 22:05 Blue Swirl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).