On Sat, Aug 21, 2010 at 1:19 PM, Markus Armbruster <arm...@redhat.com> wrote:
> Blue Swirl <blauwir...@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 <blauwir...@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.
