Rule 13.2 states: "The value of an expression and its persistent side effects
shall be the same under all permitted evaluation orders".

Eclair complains about a Rule 13.2 violations because validate_far_branch()
assigns to rc, and the entirety of commit_far_branch() is also assigned to rc.

I'm unsure that the complaint is accurate, but rewriting commit_far_branch()
to use the comma operator seems to make Eclair happy.

Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 8e14ebb35b0e..6ee64cb85987 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,

base-commit: 9b3a02e66f058ebd77db6628e3144352857bdf2b
-- 
2.39.5

On 10.05.2025 02:18, Andrew Cooper wrote:
> Rule 13.2 states: "The value of an expression and its persistent side effects
> shall be the same under all permitted evaluation orders".
> 
> Eclair complains about a Rule 13.2 violations because validate_far_branch()
> assigns to rc,

Followed by "goto", i.e. not taking the path to ...

> and the entirety of commit_far_branch() is also assigned to rc.

... that assignment. This pretty clearly looks like a tool shortcoming to
me, and I think it would be a good idea to ...

> --- 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)                        \
> +    )

... add a brief comment here clarifying why this odd a statement is used.

Jan