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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew