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

[Thomas Huth]

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>