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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 10 May 2025, Andrew Cooper wrote:
> On 07/05/2025 11:46 pm, 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.
> 
> The (mostly expanded) and reformatted expression is:
> 
> > if ( (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
> >      (rc = ({
> >              ({
> >                  if ( sizeof(dst.val) <= 4 )
> >                  {
> >                      do {
> >                          if ( __builtin_expect(!!(!(!ctxt->lma)),0) )
> >                              ASSERT();
> >                      } while ( 0 );
> >                      ({
> >                          if ( ((dst.val) > (&cs)->limit) )
> >                          {
> >                              x86_emul_hw_exception(13, mkec(13, 0, 0), 
> > ctxt);
> >                              rc = 2;
> >                              goto done;
> >                          }
> >                      });
> >                  }
> >                  else
> >                      ({
> >                          if ( (ctxt->lma && (&cs)->l
> >                                ? !(((long)(dst.val) >> 47) == 
> > ((long)(dst.val) >> 63))
> >                                : (dst.val) > (&cs)->limit) )
> >                          {
> >                              x86_emul_hw_exception(13, mkec(13, 0, 0), 
> > ctxt);
> >                              rc = 2;
> >                              goto done;
> >                          }
> >                      });
> >              });
> >              _regs.rip = (dst.val);
> >              singlestep = _regs.eflags & 0x00000100;
> >              ops->write_segment(x86_seg_cs, &cs, ctxt);
> >          })) )
> 
> And I can't identify anywhere where there is an ambiguous order of
> evaluation.
> 
> The problem with this patch is that, while the existing logic is not
> exactly great, ...
> 
> > 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;
> 
> ... this is breaking a visual layout pattern where you can always see
> the update to rc beside the "goto done".
> 
> This code is complicated enough without making it harder.
> 
> One option which might work is this:
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 3350303f8634..dab4373ece1e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -333,12 +333,14 @@ do
> {                                                                    \
>                                : (ip) > (cs)->limit, X86_EXC_GP, 0);     \
>  })
>  
> -#define commit_far_branch(cs, newip) ({                                 \
> -    validate_far_branch(cs, newip);                                     \
> -    _regs.r(ip) = (newip);                                              \
> -    singlestep = _regs.eflags & X86_EFLAGS_TF;                          \
> -    ops->write_segment(x86_seg_cs, cs, ctxt);                           \
> -})
> +#define commit_far_branch(cs, newip) (                                  \
> +        ({                                                              \
> +            validate_far_branch(cs, newip);                             \
> +            _regs.r(ip) = (newip);                                      \
> +            singlestep = _regs.eflags & X86_EFLAGS_TF;                  \
> +        }),                                                             \
> +        ops->write_segment(x86_seg_cs, cs, ctxt)                        \
> +    )
>  
>  int x86emul_get_fpu(
>      enum x86_emulate_fpu_type type,
> 
> 
> This rearranges commit_far_branch() to use the comma operator, but
> maintains the previous property that it's still overall an assignment to rc.
> 
> However, I don't have much confidence that Eclair is going to like it more.

Eclair likes it:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>