Re: [Qemu-devel] [PATCH 2/2] target-i386: Don't left shift negative constant

[Richard Henderson <rth@twiddle.net>]

On 10/23/2015 05:07 AM, Eduardo Habkost wrote:
> On Thu, Oct 01, 2015 at 02:06:36PM -0300, Eduardo Habkost wrote:
>> On Thu, Oct 01, 2015 at 11:35:52AM +1000, Richard Henderson wrote:
>>> On 09/30/2015 06:34 AM, Eduardo Habkost wrote:
>>>> Left shift of negative values is undefined behavior. Detected by clang:
>>>>    qemu/target-i386/translate.c:2423:26: runtime error:
>>>>      left shift of negative value -8
>>>>
>>>> This changes the code to reverse the sign after the left shift.
>>>>
>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> ---
>>>>   target-i386/translate.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>>>> index 8b35de1..cc59b7e 100644
>>>> --- a/target-i386/translate.c
>>>> +++ b/target-i386/translate.c
>>>> @@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s)
>>>>   {
>>>>       int i;
>>>>       gen_op_movl_A0_reg(R_ESP);
>>>> -    gen_op_addl_A0_im(-8 << s->dflag);
>>>> +    gen_op_addl_A0_im(-(8 << s->dflag));
>>>
>>> Better as -8U << s->d_flag?
>>
>> That's even more confusing to me. I wouldn't want to require other
>> people to read the C specification to find out how many type conversions
>> are happening in that statement. (Because I will have to do that, to
>> find out what's the type of "-8U").
>>
>> I would prefer an expression that doesn't involve any type conversion.
>> But you are more familiar with that code, so it's up to you.
>
> Ping? I would really like to fix this warning to be able to enable
> check-mode by default in QEMU 2.5. Are there objections to get this
> patch included?
>

None.


r~