Re: [PATCH v1 1/2] x86: x86_emulate: address violation of MISRA C Rule 13.2

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 7 May 2025, victorm.lira@xxxxxxx wrote:
> From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> 
> Rule 13.2 states: "The value of an expression and its persistent
> side effects shall be the same under all permitted evaluation orders".
> 
> The full expansion of macro "commit_far_branch" contains an assignment to
> variable "rc", which is also assigned to the result of the GNU statement
> expression which "commit_far_branch" expands to.
> 
> To avoid any dependence on the order of evaluation, the latter assignment
> is moved inside the macro.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> Signed-off-by: Victor Lira <victorm.lira@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Cc: Michal Orzel <michal.orzel@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien@xxxxxxx>
> Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> Cc: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> Cc: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
>  xen/arch/x86/x86_emulate/x86_emulate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c 
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 8e14ebb35b..7108fe7b30 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -337,7 +337,7 @@ do {                                                      
>               \
>      validate_far_branch(cs, newip);                                     \
>      _regs.r(ip) = (newip);                                              \
>      singlestep = _regs.eflags & X86_EFLAGS_TF;                          \
> -    ops->write_segment(x86_seg_cs, cs, ctxt);                           \
> +    rc = ops->write_segment(x86_seg_cs, cs, ctxt);                      \
>  })
> 
>  int x86emul_get_fpu(
> @@ -2382,7 +2382,7 @@ x86_emulate(
>               (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
>                                &src.val, op_bytes, ctxt, ops)) ||
>               (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
> -             (rc = commit_far_branch(&cs, dst.val)) )
> +             commit_far_branch(&cs, dst.val) )
>              goto done;
>          break;
> 
> @@ -2438,7 +2438,7 @@ x86_emulate(
>          _regs.eflags &= mask;
>          _regs.eflags |= (eflags & ~mask) | X86_EFLAGS_MBS;
>          if ( (rc = load_seg(x86_seg_cs, sel, 1, &cs, ctxt, ops)) ||
> -             (rc = commit_far_branch(&cs, (uint32_t)eip)) )
> +             commit_far_branch(&cs, (uint32_t)eip) )
>              goto done;
>          break;
>      }
> @@ -2557,7 +2557,7 @@ x86_emulate(
>          ASSERT(!mode_64bit());
>      far_jmp:
>          if ( (rc = load_seg(x86_seg_cs, imm2, 0, &cs, ctxt, ops)) ||
> -             (rc = commit_far_branch(&cs, imm1)) )
> +             commit_far_branch(&cs, imm1) )
>              goto done;
>          break;
> 
> --
> 2.47.0
>