db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]

target/m68k/translate.c:1323:70: warning: identical expressions on both sides of logical operator
        if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
            ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~ ^

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/m68k/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ada2a91b64..1a2f421aab 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -1321,7 +1321,8 @@ static void gen_cc_cond(DisasCompare *c, DisasContext *s, int cond)
     case 5: /* CS (C) */
         /* Some cases fold C into X.  */
         if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
-            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
+            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL ||
+            op == CC_OP_LOGIC) {
             tcond = TCG_COND_NE;
             c->v1 = QREG_CC_X;
             goto done;
-- 
2.13.3

Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
> db3d7945ae extended gen_cc_cond() for cond [6, 7, 9, 10] but misswrote [4, 5]
> 
> target/m68k/translate.c:1323:70: warning: identical expressions on both sides of logical operator
>         if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
>             op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
>             ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~    ~~~~~~~~~~~~~~~~ ^
> 
> Reported-by: Clang Static Analyzer
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/m68k/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index ada2a91b64..1a2f421aab 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -1321,7 +1321,8 @@ static void gen_cc_cond(DisasCompare *c, DisasContext *s, int cond)
>      case 5: /* CS (C) */
>          /* Some cases fold C into X.  */
>          if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
> -            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
> +            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL ||
> +            op == CC_OP_LOGIC) {

According to commit (db3d7945) introducing the incorrect copy/paste, I
don't think we need the CC_OP_LOGIC here. Logic operations never
generates X flags (whereas they can generate Z and and N).

Thanks,
Laurent

On 07/24/2017 11:54 AM, Laurent Vivier wrote:
> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index ada2a91b64..1a2f421aab 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -1321,7 +1321,8 @@ static void gen_cc_cond(DisasCompare *c, DisasContext *s, int cond)
>>       case 5: /* CS (C) */
>>           /* Some cases fold C into X.  */
>>           if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
>> -            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
>> +            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL ||
>> +            op == CC_OP_LOGIC) {
> 
> According to commit (db3d7945) introducing the incorrect copy/paste, I
> don't think we need the CC_OP_LOGIC here. Logic operations never
> generates X flags (whereas they can generate Z and and N).

Indeed, look at the comment below.  LOGIC is supposed to fall through to the 
code that produces TCG_COND_NEVER.


r~

On 07/24/2017 04:01 PM, Richard Henderson wrote:
> On 07/24/2017 11:54 AM, Laurent Vivier wrote:
>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index ada2a91b64..1a2f421aab 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -1321,7 +1321,8 @@ static void gen_cc_cond(DisasCompare *c, 
>>> DisasContext *s, int cond)
>>>       case 5: /* CS (C) */
>>>           /* Some cases fold C into X.  */
>>>           if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == 
>>> CC_OP_ADDL ||
>>> -            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
>>> +            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL ||
>>> +            op == CC_OP_LOGIC) {
>>
>> According to commit (db3d7945) introducing the incorrect copy/paste, I
>> don't think we need the CC_OP_LOGIC here. Logic operations never
>> generates X flags (whereas they can generate Z and and N).
> 
> Indeed, look at the comment below.  LOGIC is supposed to fall through to 
> the code that produces TCG_COND_NEVER.

Indeed I also miss-copypasted :)

So the correct fix would be:?

          if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
-            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
+            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL) {

Le 24/07/2017 à 21:19, Philippe Mathieu-Daudé a écrit :
> On 07/24/2017 04:01 PM, Richard Henderson wrote:
>> On 07/24/2017 11:54 AM, Laurent Vivier wrote:
>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>>> index ada2a91b64..1a2f421aab 100644
>>>> --- a/target/m68k/translate.c
>>>> +++ b/target/m68k/translate.c
>>>> @@ -1321,7 +1321,8 @@ static void gen_cc_cond(DisasCompare *c,
>>>> DisasContext *s, int cond)
>>>>       case 5: /* CS (C) */
>>>>           /* Some cases fold C into X.  */
>>>>           if (op == CC_OP_ADDB || op == CC_OP_ADDW || op ==
>>>> CC_OP_ADDL ||
>>>> -            op == CC_OP_ADDB || op == CC_OP_ADDW || op ==
>>>> CC_OP_ADDL) {
>>>> +            op == CC_OP_SUBB || op == CC_OP_SUBW || op ==
>>>> CC_OP_SUBL ||
>>>> +            op == CC_OP_LOGIC) {
>>>
>>> According to commit (db3d7945) introducing the incorrect copy/paste, I
>>> don't think we need the CC_OP_LOGIC here. Logic operations never
>>> generates X flags (whereas they can generate Z and and N).
>>
>> Indeed, look at the comment below.  LOGIC is supposed to fall through
>> to the code that produces TCG_COND_NEVER.
> 
> Indeed I also miss-copypasted :)
> 
> So the correct fix would be:?
> 
>          if (op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL ||
> -            op == CC_OP_ADDB || op == CC_OP_ADDW || op == CC_OP_ADDL) {
> +            op == CC_OP_SUBB || op == CC_OP_SUBW || op == CC_OP_SUBL) {

yes

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Laurent