Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

From: Thomas Huth <thuth@redhat.com>
To: Richard Henderson <richard.henderson@linaro.org>,
	Chen Qun <kuhn.chenqun@huawei.com>,
	qemu-devel@nongnu.org, qemu-trivial@nongnu.org
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	zhang.zhanghailiang@huawei.com, ganqixin@huawei.com,
	Euler Robot <euler.robot@huawei.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1
Date: Wed, 28 Oct 2020 17:51:45 +0100	[thread overview]
Message-ID: <9aac453d-2826-1b5e-db12-386b18f38eba@redhat.com> (raw)
In-Reply-To: <d1d59436-1a8a-7aa3-7983-4344e16ab881@linaro.org>

On 28/10/2020 16.31, Richard Henderson wrote:
> On 10/28/20 5:57 AM, Thomas Huth wrote:
>> On 28/10/2020 05.18, Chen Qun wrote:
>>> The current "#ifdef TARGET_X86_64" statement affects
>>> the compiler's determination of fall through.
>>>
>>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
>>> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>>          if (is_right) {
>>>             ^
>>> target/i386/translate.c:1782:5: note: here
>>>      case MO_32:
>>>      ^~~~
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>> ---
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target/i386/translate.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/i386/translate.c b/target/i386/translate.c
>>> index caea6f5fb1..4c353427d7 100644
>>> --- a/target/i386/translate.c
>>> +++ b/target/i386/translate.c
>>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>>>          } else {
>>>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>>>          }
>>> -        /* FALLTHRU */
>>> -#ifdef TARGET_X86_64
>>> +        /* fall through */
>>>      case MO_32:
>>> +#ifdef TARGET_X86_64
>>>          /* Concatenate the two 32-bit values and use a 64-bit shift.  */
>>>          tcg_gen_subi_tl(s->tmp0, count, 1);
>>>          if (is_right) {
>>
>> The whole code here looks a little bit fishy to me ... in case TARGET_X86_64
>> is defined, the MO_16 code falls through to MO_32 ... but in case it is not
>> defined, it falls through to the default case that comes after the #ifdef
>> block? Is this really the right thing here? If so, I think there should be
>> some additional comments explaining this behavior.
>>
>> Richard, maybe you could help to judge what is right here...?
> 
> It could definitely be rewritten, but it's not wrong as is.

Ok, thanks for the clarification! In that case:

Reviewed-by: Thomas Huth <thuth@redhat.com>