| Message ID | AANLkTi=g3NHwzT6JuU30V0iBB906PHM5du7BsQY4qfT=@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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?
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.
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>
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.
"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?
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 {
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;