Re: [PATCH v1 1/3] x86: x86_emulate: address violations of MISRA C Rule 19.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2025 03:27, Stefano Stabellini wrote:
> On Mon, 28 Apr 2025, Jan Beulich wrote:
>> On 26.04.2025 01:42, victorm.lira@xxxxxxx wrote:
>>> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>>
>>> Rule 19.1 states: "An object shall not be assigned or copied
>>> to an overlapping object". Since the "call" and "compat_call" are
>>
>> Was this taken from patch 2 without editing?
>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -526,9 +526,19 @@ static inline void put_loop_count(
>>>           */                                                             \
>>>          if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 )         \
>>>          {                                                               \
>>> +            uint64_t tmp;                                               \
>>> +                                                                        \
>>>              _regs.r(cx) = 0;                                            \
>>> -            if ( extend_si ) _regs.r(si) = _regs.esi;                   \
>>> -            if ( extend_di ) _regs.r(di) = _regs.edi;                   \
>>> +            if ( extend_si )                                            \
>>> +            {                                                           \
>>> +                tmp = _regs.esi;                                        \
>>> +                _regs.r(si) = tmp;                                      \
>>> +            }                                                           \
>>> +            if ( extend_di )                                            \
>>> +            {                                                           \
>>> +                tmp = _regs.edi;                                        \
>>> +                _regs.r(di) = tmp;                                      \
>>> +            }                                                           \
>>
>> See commit 7225f13aef03 for how we chose to address similar issues elsewhere
>> in the emulator. I think we want to be consistent there. This will then also
>> eliminate ...
>>
>>> @@ -2029,7 +2039,12 @@ x86_emulate(
>>>          switch ( op_bytes )
>>>          {
>>>          case 2: _regs.ax = (int8_t)_regs.ax; break; /* cbw */
>>> -        case 4: _regs.r(ax) = (uint32_t)(int16_t)_regs.ax; break; /* cwde 
>>> */
>>> +        case 4:
>>> +            {
>>> +                uint32_t tmp = (uint32_t)(int16_t)_regs.ax;
>>> +                _regs.r(ax) = tmp;
>>> +                break; /* cwde */
>>> +            }
>>
>> ... the odd brace placement here, as well as the inconsistency in the types
>> you used for the temporary variables (both really could have been unsigned
>> int; no need for a fixed-width type).
> 
> Is this what you have in mind?

No, and that's also not what the referenced commit did in a similar situation.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -527,8 +527,8 @@ static inline void put_loop_count(
>          if ( !amd_like(ctxt) && mode_64bit() && ad_bytes == 4 )         \
>          {                                                               \
>              _regs.r(cx) = 0;                                            \
> -            if ( extend_si ) _regs.r(si) = _regs.esi;                   \
> -            if ( extend_di ) _regs.r(di) = _regs.edi;                   \
> +            if ( extend_si ) _regs.r(si) = (uint64_t)_regs.esi;         \
> +            if ( extend_di ) _regs.r(di) = (uint64_t)_regs.edi;         \

            if ( extend_si ) _regs.r(si) = (uint32_t)_regs.r(si);       \
            if ( extend_di ) _regs.r(di) = (uint32_t)_regs.r(di);       \

After all what the rule requires is that we use _the same_ field on both sides.

Jan