[XEN PATCH 00/11] xen: address MISRA C:2012 Rule 2.1

[XEN PATCH 00/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The headline for Rule 2.1 states:
"A project shall not contain unreachable code". Violations of this rule addressed
by this patch are caused by two constructs:

1. Declarations inside switch statements, before any clause. This construct is
   allowed by the language, but is considered unreachable code, since the program
	 execution doesn't ever reach these statements, even though the variables are
	 introduced in the switch block.
2. Code following functions that are not intended to return to the caller or
   additional statements required by defensive programming practices or other
	 MISRA rules (such as a break to end each switch clause, even if that break
	 is effectively unreachable).

(1) is resolved by moving the declarations in an appropriate scope, while (2) is
addressed by adding ASSERT_UNREACHABLE() calls to signal that the code below is
intentionally unreachable, thus motivating the violation of the rule.

---
The approach taken in this series to address the violations is the outcome of
a MISRA C group meeting held 25/07/2023.

Nicola Vetrini (11):
  x86/efi: move variable declaration to address MISRA C:2012 Rule 2.1
  x86: move declarations to address MISRA C:2012 Rule 2.1
  x86/uaccess: move declarations to address MISRA C:2012 Rule 2.1
  x86emul: move variable definitions to address MISRA C:2012 Rule 2.1
  drivers/pci: move variable definitions to address MISRA C:2012 Rule
    2.1
  xen/ioreq: move variable declaration to address MISRA C:2012 Rule 2.1
  xen: address MISRA C:2012 Rule 2.1
  xen: move declarations to address MISRA C:2012 Rule 2.1
  x86/xstate: moved BUILD_BUG_ON to address MISRA C:2012 Rule 2.1
  xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1
  x86/mm: Add assertion to address MISRA C:2012 Rule 2.1

 xen/arch/x86/cpuid.c                   |  3 +--
 xen/arch/x86/domain.c                  | 23 +++++++++++------------
 xen/arch/x86/efi/efi-boot.h            |  5 ++---
 xen/arch/x86/hvm/emulate.c             |  9 ++++-----
 xen/arch/x86/hvm/hvm.c                 | 10 ++++------
 xen/arch/x86/include/asm/uaccess.h     |  6 ++++--
 xen/arch/x86/irq.c                     |  3 +--
 xen/arch/x86/mm.c                      |  1 +
 xen/arch/x86/mm/p2m-pod.c              |  1 +
 xen/arch/x86/msr.c                     |  3 +--
 xen/arch/x86/x86_emulate/0f01.c        |  7 +++----
 xen/arch/x86/x86_emulate/blk.c         | 17 ++++++++---------
 xen/arch/x86/x86_emulate/decode.c      |  3 ++-
 xen/arch/x86/x86_emulate/fpu.c         |  3 +--
 xen/arch/x86/x86_emulate/util-xen.c    |  4 ++--
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 +++++++-------
 xen/arch/x86/xstate.c                  |  3 ++-
 xen/common/compat/memory.c             |  3 +--
 xen/common/domain.c                    | 15 +++++++--------
 xen/common/ioreq.c                     |  3 +--
 xen/common/sched/core.c                |  1 +
 xen/common/shutdown.c                  | 18 ++++++++++++------
 xen/drivers/passthrough/pci.c          | 10 ++++------
 23 files changed, 81 insertions(+), 84 deletions(-)

--
2.34.1

[XEN PATCH 01/11] x86/efi: move variable declaration to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The variable declaration is moved where it's actually used, rather
than being declared in the switch before any clause, thus being
classified as unreachable code.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/efi/efi-boot.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 92f4cfe8bd..b00441b1a2 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -390,8 +390,6 @@ static void __init efi_arch_edd(void)
         {
             switch ( DevicePathType(devp.DevPath) )
             {
-                const u8 *p;
-
             case ACPI_DEVICE_PATH:
                 if ( state != root || boot_edd_info_nr > EDD_INFO_MAX )
                     break;
@@ -463,7 +461,8 @@ static void __init efi_arch_edd(void)
                 params->device_path_info_length =
                     sizeof(struct edd_device_params) -
                     offsetof(struct edd_device_params, key);
-                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
+                for ( const u8 *p = (const u8 *)&params->key;
+                      p < &params->checksum; ++p )
                     params->checksum -= *p;
                 break;
             case MEDIA_DEVICE_PATH:
-- 
2.34.1

[XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Variable declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the declarations are moved in the smallest enclosing
scope, near other variable definitions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/cpuid.c  |  3 +--
 xen/arch/x86/domain.c | 23 +++++++++++------------
 xen/arch/x86/irq.c    |  3 +--
 xen/arch/x86/msr.c    |  3 +--
 4 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 455a09b2dd..90e1370e90 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
 {
     const struct domain *d = v->domain;
     const struct cpu_policy *p = d->arch.cpu_policy;
+    const struct cpu_user_regs *regs;
 
     *res = EMPTY_LEAF;
 
@@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
      */
     switch ( leaf )
     {
-        const struct cpu_user_regs *regs;
-
     case 0x1:
         /* TODO: Rework topology logic. */
         res->b &= 0x00ffffffu;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5f66c2ae33..015f7b14ab 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d)
 {
     int ret;
     struct vcpu *v;
+    enum {
+        PROG_iommu_pagetables = 1,
+        PROG_shared,
+        PROG_paging,
+        PROG_vcpu_pagetables,
+        PROG_xen,
+        PROG_l4,
+        PROG_l3,
+        PROG_l2,
+        PROG_done,
+    };
 
     BUG_ON(!cpumask_empty(d->dirty_cpumask));
 
@@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d)
 #define PROGRESS(x)                                                     \
         d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
 
-        enum {
-            PROG_iommu_pagetables = 1,
-            PROG_shared,
-            PROG_paging,
-            PROG_vcpu_pagetables,
-            PROG_xen,
-            PROG_l4,
-            PROG_l3,
-            PROG_l2,
-            PROG_done,
-        };
-
     case 0:
         ret = pci_release_devices(d);
         if ( ret )
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 6abfd81621..4fd0cc163d 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1135,6 +1135,7 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
     struct irq_desc *desc = data;
     unsigned int i, irq = desc - irq_desc;
     irq_guest_action_t *action;
+    cpumask_t *cpu_eoi_map;
 
     spin_lock_irq(&desc->lock);
     
@@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
 
     switch ( action->ack_type )
     {
-        cpumask_t *cpu_eoi_map;
-
     case ACKTYPE_UNMASK:
         if ( desc->handler->end )
             desc->handler->end(desc, 0);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index ecf126566d..0e61e0fe4e 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -335,11 +335,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     const struct cpu_policy *cp = d->arch.cpu_policy;
     struct vcpu_msrs *msrs = v->arch.msrs;
     int ret = X86EMUL_OKAY;
+    uint64_t rsvd;
 
     switch ( msr )
     {
-        uint64_t rsvd;
-
         /* Read-only */
     case MSR_IA32_PLATFORM_ID:
     case MSR_CORE_CAPABILITIES:
-- 
2.34.1

[XEN PATCH 03/11] x86/uaccess: move declarations to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The variable declarations inside macros {put,get}_unsafe_size are
considered unreachable code, as they appear in a switch statement, before
any clause. They are moved in the enclosing scope to resolve the violation.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/include/asm/uaccess.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
index 684fccd95c..a2aaab1e87 100644
--- a/xen/arch/x86/include/asm/uaccess.h
+++ b/xen/arch/x86/include/asm/uaccess.h
@@ -191,11 +191,12 @@ struct __large_struct { unsigned long buf[100]; };
 
 #define put_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
+    long dummy_;                                                           \
+                                                                           \
     retval = 0;                                                            \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
-    long dummy_;                                                           \
     case 1:                                                                \
         put_unsafe_asm(x, ptr, grd, retval, "b", "b", "iq", errret);       \
         break;                                                             \
@@ -218,11 +219,12 @@ do {                                                                       \
 
 #define get_unsafe_size(x, ptr, size, grd, retval, errret)                 \
 do {                                                                       \
+    long dummy_;                                                           \
+                                                                           \
     retval = 0;                                                            \
     stac();                                                                \
     switch ( size )                                                        \
     {                                                                      \
-    long dummy_;                                                           \
     case 1: get_unsafe_asm(x, ptr, grd, retval, "b", "=q", errret); break; \
     case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); break; \
     case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); break; \
-- 
2.34.1

[XEN PATCH 04/11] x86emul: move variable definitions to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Variable declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the variable declarations are moved in the only clause
scope that uses it.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/hvm/emulate.c             |  9 ++++-----
 xen/arch/x86/hvm/hvm.c                 | 10 ++++------
 xen/arch/x86/x86_emulate/0f01.c        |  7 +++----
 xen/arch/x86/x86_emulate/blk.c         | 17 ++++++++---------
 xen/arch/x86/x86_emulate/decode.c      |  3 ++-
 xen/arch/x86/x86_emulate/fpu.c         |  3 +--
 xen/arch/x86/x86_emulate/util-xen.c    |  4 ++--
 xen/arch/x86/x86_emulate/x86_emulate.c | 14 +++++++-------
 8 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 75ee98a73b..2261e8655b 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1994,6 +1994,7 @@ static int cf_check hvmemul_rep_stos(
     bool_t df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
     int rc = hvmemul_virtual_to_linear(seg, offset, bytes_per_rep, reps,
                                        hvm_access_write, hvmemul_ctxt, &addr);
+    char *buf;
 
     if ( rc != X86EMUL_OKAY )
         return rc;
@@ -2025,7 +2026,6 @@ static int cf_check hvmemul_rep_stos(
     switch ( p2mt )
     {
         unsigned long bytes;
-        char *buf;
 
     default:
         /* Allocate temporary buffer. */
@@ -2289,16 +2289,15 @@ static int cf_check hvmemul_cache_op(
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
     uint32_t pfec = PFEC_page_present;
+    unsigned long addr;
+    int rc;
+    void *mapping;
 
     if ( !cache_flush_permitted(current->domain) )
         return X86EMUL_OKAY;
 
     switch ( op )
     {
-        unsigned long addr;
-        int rc;
-        void *mapping;
-
     case x86emul_clflush:
     case x86emul_clflushopt:
     case x86emul_clwb:
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2180abeb33..4170343b34 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1494,10 +1494,10 @@ static int cf_check hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
 
     for ( i = 0; i < ctxt->count; ++i )
     {
+        int rc;
+
         switch ( ctxt->msr[i].index )
         {
-            int rc;
-
         case MSR_SPEC_CTRL:
         case MSR_INTEL_MISC_FEATURES_ENABLES:
         case MSR_PKRS:
@@ -3522,6 +3522,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     struct domain *d = v->domain;
     uint64_t *var_range_base, *fixed_range_base;
     int ret;
+    unsigned int index;
 
     var_range_base = (uint64_t *)v->arch.hvm.mtrr.var_ranges;
     fixed_range_base = (uint64_t *)v->arch.hvm.mtrr.fixed_ranges;
@@ -3533,8 +3534,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
 
     switch ( msr )
     {
-        unsigned int index;
-
     case MSR_EFER:
         *msr_content = v->arch.hvm.guest_efer;
         break;
@@ -3636,6 +3635,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
     struct vcpu *v = current;
     struct domain *d = v->domain;
     int ret;
+    unsigned int index;
 
     HVMTRACE_3D(MSR_WRITE, msr,
                (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
@@ -3668,8 +3668,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
 
     switch ( msr )
     {
-        unsigned int index;
-
     case MSR_EFER:
         if ( hvm_set_efer(msr_content) )
            return X86EMUL_EXCEPTION;
diff --git a/xen/arch/x86/x86_emulate/0f01.c b/xen/arch/x86/x86_emulate/0f01.c
index ba43fc394b..22a14b12c3 100644
--- a/xen/arch/x86/x86_emulate/0f01.c
+++ b/xen/arch/x86/x86_emulate/0f01.c
@@ -24,13 +24,12 @@ int x86emul_0f01(struct x86_emulate_state *s,
 {
     enum x86_segment seg = (s->modrm_reg & 1) ? x86_seg_idtr : x86_seg_gdtr;
     int rc;
+    unsigned long base, limit, cr0, cr0w, cr4;
+    struct segment_register sreg;
+    uint64_t msr_val;
 
     switch ( s->modrm )
     {
-        unsigned long base, limit, cr0, cr0w, cr4;
-        struct segment_register sreg;
-        uint64_t msr_val;
-
     case 0xc6:
         switch ( s->vex.pfx )
         {
diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
index e790f4f900..f2956c0d52 100644
--- a/xen/arch/x86/x86_emulate/blk.c
+++ b/xen/arch/x86/x86_emulate/blk.c
@@ -26,19 +26,18 @@ int x86_emul_blk(
     struct x86_emulate_ctxt *ctxt)
 {
     int rc = X86EMUL_OKAY;
-
-    switch ( s->blk )
-    {
-        bool zf;
+    bool zf;
 #ifndef X86EMUL_NO_FPU
+    struct {
+        struct x87_env32 env;
         struct {
-            struct x87_env32 env;
-            struct {
-               uint8_t bytes[10];
-            } freg[8];
-        } fpstate;
+           uint8_t bytes[10];
+        } freg[8];
+    } fpstate;
 #endif
 
+    switch ( s->blk )
+    {
         /*
          * Throughout this switch(), memory clobbers are used to compensate
          * that other operands may not properly express the (full) memory
diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
index f58ca3984e..daebf3a9bd 100644
--- a/xen/arch/x86/x86_emulate/decode.c
+++ b/xen/arch/x86/x86_emulate/decode.c
@@ -1758,9 +1758,9 @@ int x86emul_decode(struct x86_emulate_state *s,
     /* Fetch the immediate operand, if present. */
     switch ( d & SrcMask )
     {
+    case SrcImm: {
         unsigned int bytes;
 
-    case SrcImm:
         if ( !(d & ByteOp) )
         {
             if ( mode_64bit() && !amd_like(ctxt) &&
@@ -1785,6 +1785,7 @@ int x86emul_decode(struct x86_emulate_state *s,
     case SrcImm16:
         s->imm1 = insn_fetch_type(uint16_t);
         break;
+		}
     }
 
     ctxt->opcode = opcode;
diff --git a/xen/arch/x86/x86_emulate/fpu.c b/xen/arch/x86/x86_emulate/fpu.c
index 480d879657..002d5e1253 100644
--- a/xen/arch/x86/x86_emulate/fpu.c
+++ b/xen/arch/x86/x86_emulate/fpu.c
@@ -84,11 +84,10 @@ int x86emul_fpu(struct x86_emulate_state *s,
     uint8_t b;
     int rc;
     struct x86_emulate_stub stub = {};
+    unsigned long dummy;
 
     switch ( b = ctxt->opcode )
     {
-        unsigned long dummy;
-
     case 0x9b:  /* wait/fwait */
         host_and_vcpu_must_have(fpu);
         get_fpu(X86EMUL_FPU_wait);
diff --git a/xen/arch/x86/x86_emulate/util-xen.c b/xen/arch/x86/x86_emulate/util-xen.c
index 5e90818010..7ab2ac712f 100644
--- a/xen/arch/x86/x86_emulate/util-xen.c
+++ b/xen/arch/x86/x86_emulate/util-xen.c
@@ -77,10 +77,10 @@ bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
 bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
                                     const struct x86_emulate_ctxt *ctxt)
 {
+    unsigned int ext;
+
     switch ( ctxt->opcode )
     {
-        unsigned int ext;
-
     case X86EMUL_OPC(0x0f, 0x01):
         if ( x86_insn_modrm(s, NULL, &ext) >= 0
              && (ext & 5) == 4 ) /* SMSW / LMSW */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e88245eae9..d6c04fd5f3 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1479,15 +1479,15 @@ x86_emulate(
         break;
     }
 
+    enum x86_segment seg;
+    struct segment_register cs, sreg;
+    struct cpuid_leaf leaf;
+    uint64_t msr_val;
+    unsigned int i, n;
+    unsigned long dummy;
+
     switch ( ctxt->opcode )
     {
-        enum x86_segment seg;
-        struct segment_register cs, sreg;
-        struct cpuid_leaf leaf;
-        uint64_t msr_val;
-        unsigned int i, n;
-        unsigned long dummy;
-
     case 0x00: case 0x01: add: /* add reg,mem */
         if ( ops->rmw && dst.type == OP_MEM )
             state->rmw = rmw_add;
-- 
2.34.1

[XEN PATCH 05/11] drivers/pci: move variable definitions to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Variable declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the variable declarations are moved in the smallest enclosing
scope, near other variable definitions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/drivers/passthrough/pci.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 33452791a8..3f5fa5beef 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -315,6 +315,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     struct pci_dev *pdev;
     unsigned int pos;
     int rc;
+    unsigned int cap, sec_bus, sub_bus;
 
     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
         if ( pdev->bus == bus && pdev->devfn == devfn )
@@ -343,8 +344,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
     /* update bus2bridge */
     switch ( pdev->type = pdev_type(pseg->nr, bus, devfn) )
     {
-        unsigned int cap, sec_bus, sub_bus;
-
         case DEV_TYPE_PCIe2PCI_BRIDGE:
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pdev->sbdf, PCI_SECONDARY_BUS);
@@ -424,11 +423,11 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 
 static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
 {
+    unsigned int sec_bus, sub_bus;
+
     /* update bus2bridge */
     switch ( pdev->type )
     {
-        unsigned int sec_bus, sub_bus;
-
         case DEV_TYPE_PCIe2PCI_BRIDGE:
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pdev->sbdf, PCI_SECONDARY_BUS);
@@ -1555,11 +1554,10 @@ int iommu_do_pci_domctl(
     u8 bus, devfn;
     int ret = 0;
     uint32_t machine_sbdf;
+    unsigned int flags;
 
     switch ( domctl->cmd )
     {
-        unsigned int flags;
-
     case XEN_DOMCTL_get_device_group:
     {
         u32 max_sdevs;
-- 
2.34.1

[XEN PATCH 06/11] xen/ioreq: move variable declaration to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Variable declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the variable declarations are moved in the smallest enclosing
scope, near other variable definitions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/ioreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 7cb717f7a2..5044f43b81 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -1111,6 +1111,7 @@ struct ioreq_server *ioreq_server_select(struct domain *d,
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct rangeset *r;
+        unsigned long start, end;
 
         if ( !s->enabled )
             continue;
@@ -1119,8 +1120,6 @@ struct ioreq_server *ioreq_server_select(struct domain *d,
 
         switch ( type )
         {
-            unsigned long start, end;
-
         case XEN_DMOP_IO_RANGE_PORT:
             start = addr;
             end = start + p->size - 1;
-- 
2.34.1

[XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Rule 2.1 states: "A project shall not contain unreachable code".

The functions
- machine_halt
- maybe_reboot
- machine_restart
are not supposed to return, hence the following break statement
is marked as intentionally unreachable with the ASSERT_UNREACHABLE()
macro to justify the violation of the rule.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/shutdown.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index a933ee001e..88f735a30e 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -38,39 +38,45 @@ void hwdom_shutdown(u8 reason)
         printk("Hardware Dom%u halted: halting machine\n",
                hardware_domain->domain_id);
         machine_halt();
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
 
     case SHUTDOWN_crash:
         debugger_trap_immediate();
         printk("Hardware Dom%u crashed: ", hardware_domain->domain_id);
         kexec_crash(CRASHREASON_HWDOM);
         maybe_reboot();
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
 
     case SHUTDOWN_reboot:
         printk("Hardware Dom%u shutdown: rebooting machine\n",
                hardware_domain->domain_id);
         machine_restart(0);
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
 
     case SHUTDOWN_watchdog:
         printk("Hardware Dom%u shutdown: watchdog rebooting machine\n",
                hardware_domain->domain_id);
         kexec_crash(CRASHREASON_WATCHDOG);
         machine_restart(0);
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
 
     case SHUTDOWN_soft_reset:
         printk("Hardware domain %d did unsupported soft reset, rebooting.\n",
                hardware_domain->domain_id);
         machine_restart(0);
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
 
     default:
         printk("Hardware Dom%u shutdown (unknown reason %u): ",
                hardware_domain->domain_id, reason);
         maybe_reboot();
-        break; /* not reached */
+        ASSERT_UNREACHABLE();
+        break;
     }
 }  
 
-- 
2.34.1

[XEN PATCH 08/11] xen: move declarations to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the variable declarations are moved in the smallest enclosing
scope, near other variable definitions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/common/compat/memory.c |  3 +--
 xen/common/domain.c        | 15 +++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 8ca63ceda6..d4c4204119 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -85,13 +85,12 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat)
             struct compat_mem_access_op mao;
             struct compat_mem_acquire_resource mar;
         } cmp;
+        xen_pfn_t *space;
 
         set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
         split = 0;
         switch ( op )
         {
-            xen_pfn_t *space;
-
         case XENMEM_increase_reservation:
         case XENMEM_decrease_reservation:
         case XENMEM_populate_physmap:
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 304aa04fa6..e3aeaf059d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -401,6 +401,13 @@ static int domain_teardown(struct domain *d)
 {
     struct vcpu *v;
     int rc;
+    enum {
+        PROG_none,
+        PROG_gnttab_mappings,
+        PROG_vcpu_teardown,
+        PROG_arch_teardown,
+        PROG_done,
+    };
 
     BUG_ON(!d->is_dying);
 
@@ -435,14 +442,6 @@ static int domain_teardown(struct domain *d)
     case PROG_vcpu_ ## x:                       \
         v = d->teardown.vcpu
 
-        enum {
-            PROG_none,
-            PROG_gnttab_mappings,
-            PROG_vcpu_teardown,
-            PROG_arch_teardown,
-            PROG_done,
-        };
-
     case PROG_none:
         BUILD_BUG_ON(PROG_none != 0);
 
-- 
2.34.1

[XEN PATCH 09/11] x86/xstate: moved BUILD_BUG_ON to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Variable declarations between a switch statement guard and before
any case label are unreachable code, and hence violate Rule 2.1:
"A project shall not contain unreachable code".

Therefore the variable declarations are moved in the smallest enclosing
scope, near other variable definitions.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/xstate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 92496f3795..cb2b9e720c 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -396,9 +396,10 @@ void xrstor(struct vcpu *v, uint64_t mask)
      */
     for ( prev_faults = faults = 0; ; prev_faults = faults )
     {
+        BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
+
         switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
         {
-            BUILD_BUG_ON(sizeof(faults) != 4); /* Clang doesn't support %z in asm. */
 #define _xrstor(insn) \
         asm volatile ( "1: .byte " insn "\n" \
                        "3:\n" \
-- 
2.34.1

[XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The break statement after the return statement is definitely unreachable.
As such, an call to the ASSERT_UNREACHABLE() macro is added to signal
the intentionality of such construct.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
The break in the clause is mandated by Required Rule 16.3, which is
not yet an accepted rule for Xen, but may be in the future.
---
 xen/common/sched/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 022f548652..fcee902b4e 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
         /* fallthrough */
     case TASKLET_enqueued|TASKLET_scheduled:
         return true;
+        ASSERT_UNREACHABLE();
         break;
     case TASKLET_scheduled:
         clear_bit(_TASKLET_scheduled, tasklet_work);
-- 
2.34.1

[XEN PATCH 11/11] x86/mm: Add assertion to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

The ASSERT_UNREACHABLE() assertion is added before a definitely
unreachable return statement to address MISRA C:2012 Rule 2.1,
because the explicit return from a non-void function is a defensive
coding measure, and thus intended to be unreachable.

No functional changes.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/arch/x86/mm.c         | 1 +
 xen/arch/x86/mm/p2m-pod.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index be2b10a391..ebd4f3827a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return subarch_memory_op(cmd, arg);
     }
 
+    ASSERT_UNREACHABLE();
     return 0;
 }
 
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 9969eb45fa..cd5501217f 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
     }
 
     return;
+    ASSERT_UNREACHABLE();
 
 out_unmap:
     /*
-- 
2.34.1

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Andrew Cooper]

On 02/08/2023 3:38 pm, Nicola Vetrini wrote:
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5f66c2ae33..015f7b14ab 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d)
>  {
>      int ret;
>      struct vcpu *v;
> +    enum {
> +        PROG_iommu_pagetables = 1,
> +        PROG_shared,
> +        PROG_paging,
> +        PROG_vcpu_pagetables,
> +        PROG_xen,
> +        PROG_l4,
> +        PROG_l3,
> +        PROG_l2,
> +        PROG_done,
> +    };
>  
>      BUG_ON(!cpumask_empty(d->dirty_cpumask));
>  
> @@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d)
>  #define PROGRESS(x)                                                     \
>          d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
>  
> -        enum {
> -            PROG_iommu_pagetables = 1,
> -            PROG_shared,
> -            PROG_paging,
> -            PROG_vcpu_pagetables,
> -            PROG_xen,
> -            PROG_l4,
> -            PROG_l3,
> -            PROG_l2,
> -            PROG_done,
> -        };
> -
>      case 0:
>          ret = pci_release_devices(d);
>          if ( ret )

Why does this get moved?  There's no code (reachable or unreachable) in
there.

This is very subtle logic to start with, and you're moving one part of
it away from the comment explaining how the magic works.

~Andrew

Re: [XEN PATCH 01/11] x86/efi: move variable declaration to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> The variable declaration is moved where it's actually used, rather
> than being declared in the switch before any clause, thus being
> classified as unreachable code.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/efi/efi-boot.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 92f4cfe8bd..b00441b1a2 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -390,8 +390,6 @@ static void __init efi_arch_edd(void)
>          {
>              switch ( DevicePathType(devp.DevPath) )
>              {
> -                const u8 *p;
> -
>              case ACPI_DEVICE_PATH:
>                  if ( state != root || boot_edd_info_nr > EDD_INFO_MAX )
>                      break;
> @@ -463,7 +461,8 @@ static void __init efi_arch_edd(void)
>                  params->device_path_info_length =
>                      sizeof(struct edd_device_params) -
>                      offsetof(struct edd_device_params, key);
> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
> +                for ( const u8 *p = (const u8 *)&params->key;
> +                      p < &params->checksum; ++p )

In Xen we don't mix declaration and code. So the following is not
something we use:

  for (int i = 0; i < 10; i++)

I think you'd have to introduce another block under case
MESSAGING_DEVICE_PATH so that you can moved const u8 *p there


>                      params->checksum -= *p;
>                  break;
>              case MEDIA_DEVICE_PATH:
> -- 
> 2.34.1
> 

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Variable declarations between a switch statement guard and before
> any case label are unreachable code, and hence violate Rule 2.1:
> "A project shall not contain unreachable code".
> 
> Therefore the declarations are moved in the smallest enclosing
> scope, near other variable definitions.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/cpuid.c  |  3 +--
>  xen/arch/x86/domain.c | 23 +++++++++++------------
>  xen/arch/x86/irq.c    |  3 +--
>  xen/arch/x86/msr.c    |  3 +--
>  4 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index 455a09b2dd..90e1370e90 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>  {
>      const struct domain *d = v->domain;
>      const struct cpu_policy *p = d->arch.cpu_policy;
> +    const struct cpu_user_regs *regs;
>  
>      *res = EMPTY_LEAF;
>  
> @@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>       */
>      switch ( leaf )
>      {
> -        const struct cpu_user_regs *regs;
> -
>      case 0x1:
>          /* TODO: Rework topology logic. */
>          res->b &= 0x00ffffffu;
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 5f66c2ae33..015f7b14ab 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2268,6 +2268,17 @@ int domain_relinquish_resources(struct domain *d)
>  {
>      int ret;
>      struct vcpu *v;
> +    enum {
> +        PROG_iommu_pagetables = 1,
> +        PROG_shared,
> +        PROG_paging,
> +        PROG_vcpu_pagetables,
> +        PROG_xen,
> +        PROG_l4,
> +        PROG_l3,
> +        PROG_l2,
> +        PROG_done,
> +    };
>  
>      BUG_ON(!cpumask_empty(d->dirty_cpumask));
>  
> @@ -2291,18 +2302,6 @@ int domain_relinquish_resources(struct domain *d)
>  #define PROGRESS(x)                                                     \
>          d->arch.rel_priv = PROG_ ## x; /* Fallthrough */ case PROG_ ## x
>  
> -        enum {
> -            PROG_iommu_pagetables = 1,
> -            PROG_shared,
> -            PROG_paging,
> -            PROG_vcpu_pagetables,
> -            PROG_xen,
> -            PROG_l4,
> -            PROG_l3,
> -            PROG_l2,
> -            PROG_done,
> -        };
> -
>      case 0:
>          ret = pci_release_devices(d);
>          if ( ret )
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 6abfd81621..4fd0cc163d 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1135,6 +1135,7 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>      struct irq_desc *desc = data;
>      unsigned int i, irq = desc - irq_desc;
>      irq_guest_action_t *action;
> +    cpumask_t *cpu_eoi_map;
>  
>      spin_lock_irq(&desc->lock);
>      
> @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>  
>      switch ( action->ack_type )
>      {
> -        cpumask_t *cpu_eoi_map;

It is only used by case ACKTYPE_EOI so it can be moved there (with a new
block):


    case ACKTYPE_EOI:
    {
        cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
        cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
        spin_unlock_irq(&desc->lock);
        on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
        return;
    }
    }


>      case ACKTYPE_UNMASK:
>          if ( desc->handler->end )
>              desc->handler->end(desc, 0);
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index ecf126566d..0e61e0fe4e 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -335,11 +335,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>      const struct cpu_policy *cp = d->arch.cpu_policy;
>      struct vcpu_msrs *msrs = v->arch.msrs;
>      int ret = X86EMUL_OKAY;
> +    uint64_t rsvd;
>  
>      switch ( msr )
>      {
> -        uint64_t rsvd;
> -

It is only used by case MSR_INTEL_MISC_FEATURES_ENABLES so it can be
moved there


>          /* Read-only */
>      case MSR_IA32_PLATFORM_ID:
>      case MSR_CORE_CAPABILITIES:
> -- 
> 2.34.1
> 

Re: [XEN PATCH 03/11] x86/uaccess: move declarations to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> The variable declarations inside macros {put,get}_unsafe_size are
> considered unreachable code, as they appear in a switch statement, before
> any clause. They are moved in the enclosing scope to resolve the violation.
> 
> No functional changes.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/x86/include/asm/uaccess.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/uaccess.h b/xen/arch/x86/include/asm/uaccess.h
> index 684fccd95c..a2aaab1e87 100644
> --- a/xen/arch/x86/include/asm/uaccess.h
> +++ b/xen/arch/x86/include/asm/uaccess.h
> @@ -191,11 +191,12 @@ struct __large_struct { unsigned long buf[100]; };
>  
>  #define put_unsafe_size(x, ptr, size, grd, retval, errret)                 \
>  do {                                                                       \
> +    long dummy_;                                                           \
> +                                                                           \
>      retval = 0;                                                            \
>      stac();                                                                \
>      switch ( size )                                                        \
>      {                                                                      \
> -    long dummy_;                                                           \
>      case 1:                                                                \
>          put_unsafe_asm(x, ptr, grd, retval, "b", "b", "iq", errret);       \
>          break;                                                             \
> @@ -218,11 +219,12 @@ do {                                                                       \
>  
>  #define get_unsafe_size(x, ptr, size, grd, retval, errret)                 \
>  do {                                                                       \
> +    long dummy_;                                                           \
> +                                                                           \
>      retval = 0;                                                            \
>      stac();                                                                \
>      switch ( size )                                                        \
>      {                                                                      \
> -    long dummy_;                                                           \
>      case 1: get_unsafe_asm(x, ptr, grd, retval, "b", "=q", errret); break; \
>      case 2: get_unsafe_asm(x, ptr, grd, retval, "w", "=r", errret); break; \
>      case 4: get_unsafe_asm(x, ptr, grd, retval, "k", "=r", errret); break; \
> -- 
> 2.34.1
> 

Re: [XEN PATCH 04/11] x86emul: move variable definitions to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Variable declarations between a switch statement guard and before
> any case label are unreachable code, and hence violate Rule 2.1:
> "A project shall not contain unreachable code".
> 
> Therefore the variable declarations are moved in the only clause
> scope that uses it.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/arch/x86/hvm/emulate.c             |  9 ++++-----
>  xen/arch/x86/hvm/hvm.c                 | 10 ++++------
>  xen/arch/x86/x86_emulate/0f01.c        |  7 +++----
>  xen/arch/x86/x86_emulate/blk.c         | 17 ++++++++---------
>  xen/arch/x86/x86_emulate/decode.c      |  3 ++-
>  xen/arch/x86/x86_emulate/fpu.c         |  3 +--
>  xen/arch/x86/x86_emulate/util-xen.c    |  4 ++--
>  xen/arch/x86/x86_emulate/x86_emulate.c | 14 +++++++-------
>  8 files changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 75ee98a73b..2261e8655b 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1994,6 +1994,7 @@ static int cf_check hvmemul_rep_stos(
>      bool_t df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
>      int rc = hvmemul_virtual_to_linear(seg, offset, bytes_per_rep, reps,
>                                         hvm_access_write, hvmemul_ctxt, &addr);
> +    char *buf;
>  
>      if ( rc != X86EMUL_OKAY )
>          return rc;
> @@ -2025,7 +2026,6 @@ static int cf_check hvmemul_rep_stos(
>      switch ( p2mt )
>      {
>          unsigned long bytes;
> -        char *buf;

why can "bytes" where it is?
Bith buf and bytes could be declared under "default" with a new block


>      default:
>          /* Allocate temporary buffer. */
> @@ -2289,16 +2289,15 @@ static int cf_check hvmemul_cache_op(
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>      uint32_t pfec = PFEC_page_present;
> +    unsigned long addr;
> +    int rc;
> +    void *mapping;
>  
>      if ( !cache_flush_permitted(current->domain) )
>          return X86EMUL_OKAY;
>  
>      switch ( op )
>      {
> -        unsigned long addr;
> -        int rc;
> -        void *mapping;

These three could be...


>      case x86emul_clflush:
>      case x86emul_clflushopt:
>      case x86emul_clwb:

... here in a new block



> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2180abeb33..4170343b34 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1494,10 +1494,10 @@ static int cf_check hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>  
>      for ( i = 0; i < ctxt->count; ++i )
>      {
> +        int rc;
> +
>          switch ( ctxt->msr[i].index )
>          {
> -            int rc;
> -
>          case MSR_SPEC_CTRL:
>          case MSR_INTEL_MISC_FEATURES_ENABLES:
>          case MSR_PKRS:
> @@ -3522,6 +3522,7 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>      struct domain *d = v->domain;
>      uint64_t *var_range_base, *fixed_range_base;
>      int ret;
> +    unsigned int index;
>  
>      var_range_base = (uint64_t *)v->arch.hvm.mtrr.var_ranges;
>      fixed_range_base = (uint64_t *)v->arch.hvm.mtrr.fixed_ranges;
> @@ -3533,8 +3534,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>  
>      switch ( msr )
>      {
> -        unsigned int index;
> -
>      case MSR_EFER:
>          *msr_content = v->arch.hvm.guest_efer;
>          break;
> @@ -3636,6 +3635,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
>      struct vcpu *v = current;
>      struct domain *d = v->domain;
>      int ret;
> +    unsigned int index;
>  
>      HVMTRACE_3D(MSR_WRITE, msr,
>                 (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
> @@ -3668,8 +3668,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
>  
>      switch ( msr )
>      {
> -        unsigned int index;
> -
>      case MSR_EFER:
>          if ( hvm_set_efer(msr_content) )
>             return X86EMUL_EXCEPTION;
> diff --git a/xen/arch/x86/x86_emulate/0f01.c b/xen/arch/x86/x86_emulate/0f01.c
> index ba43fc394b..22a14b12c3 100644
> --- a/xen/arch/x86/x86_emulate/0f01.c
> +++ b/xen/arch/x86/x86_emulate/0f01.c
> @@ -24,13 +24,12 @@ int x86emul_0f01(struct x86_emulate_state *s,
>  {
>      enum x86_segment seg = (s->modrm_reg & 1) ? x86_seg_idtr : x86_seg_gdtr;
>      int rc;
> +    unsigned long base, limit, cr0, cr0w, cr4;
> +    struct segment_register sreg;
> +    uint64_t msr_val;

base and limit can go under case 0xfc

cr0 and cr0w can go under case GRP7_ALL(6)




>      switch ( s->modrm )
>      {
> -        unsigned long base, limit, cr0, cr0w, cr4;
> -        struct segment_register sreg;
> -        uint64_t msr_val;
> -
>      case 0xc6:
>          switch ( s->vex.pfx )
>          {
> diff --git a/xen/arch/x86/x86_emulate/blk.c b/xen/arch/x86/x86_emulate/blk.c
> index e790f4f900..f2956c0d52 100644
> --- a/xen/arch/x86/x86_emulate/blk.c
> +++ b/xen/arch/x86/x86_emulate/blk.c
> @@ -26,19 +26,18 @@ int x86_emul_blk(
>      struct x86_emulate_ctxt *ctxt)
>  {
>      int rc = X86EMUL_OKAY;
> -
> -    switch ( s->blk )
> -    {
> -        bool zf;
> +    bool zf;
>  #ifndef X86EMUL_NO_FPU
> +    struct {
> +        struct x87_env32 env;
>          struct {
> -            struct x87_env32 env;
> -            struct {
> -               uint8_t bytes[10];
> -            } freg[8];
> -        } fpstate;
> +           uint8_t bytes[10];
> +        } freg[8];
> +    } fpstate;
>  #endif
>  
> +    switch ( s->blk )
> +    {
>          /*
>           * Throughout this switch(), memory clobbers are used to compensate
>           * that other operands may not properly express the (full) memory
> diff --git a/xen/arch/x86/x86_emulate/decode.c b/xen/arch/x86/x86_emulate/decode.c
> index f58ca3984e..daebf3a9bd 100644
> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -1758,9 +1758,9 @@ int x86emul_decode(struct x86_emulate_state *s,
>      /* Fetch the immediate operand, if present. */
>      switch ( d & SrcMask )
>      {
> +    case SrcImm: {

The Xen coding style is:

    case SrcImm:
    {

Also, this change looks wrong?


>          unsigned int bytes;
>  
> -    case SrcImm:
>          if ( !(d & ByteOp) )
>          {
>              if ( mode_64bit() && !amd_like(ctxt) &&
> @@ -1785,6 +1785,7 @@ int x86emul_decode(struct x86_emulate_state *s,
>      case SrcImm16:
>          s->imm1 = insn_fetch_type(uint16_t);
>          break;
> +		}
>      }
>  
>      ctxt->opcode = opcode;
> diff --git a/xen/arch/x86/x86_emulate/fpu.c b/xen/arch/x86/x86_emulate/fpu.c
> index 480d879657..002d5e1253 100644
> --- a/xen/arch/x86/x86_emulate/fpu.c
> +++ b/xen/arch/x86/x86_emulate/fpu.c
> @@ -84,11 +84,10 @@ int x86emul_fpu(struct x86_emulate_state *s,
>      uint8_t b;
>      int rc;
>      struct x86_emulate_stub stub = {};
> +    unsigned long dummy;
>  
>      switch ( b = ctxt->opcode )
>      {
> -        unsigned long dummy;
> -
>      case 0x9b:  /* wait/fwait */
>          host_and_vcpu_must_have(fpu);
>          get_fpu(X86EMUL_FPU_wait);
> diff --git a/xen/arch/x86/x86_emulate/util-xen.c b/xen/arch/x86/x86_emulate/util-xen.c
> index 5e90818010..7ab2ac712f 100644
> --- a/xen/arch/x86/x86_emulate/util-xen.c
> +++ b/xen/arch/x86/x86_emulate/util-xen.c
> @@ -77,10 +77,10 @@ bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
>  bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
>                                      const struct x86_emulate_ctxt *ctxt)
>  {
> +    unsigned int ext;
> +
>      switch ( ctxt->opcode )
>      {
> -        unsigned int ext;

This can go under case X86EMUL_OPC with a new block


>      case X86EMUL_OPC(0x0f, 0x01):
>          if ( x86_insn_modrm(s, NULL, &ext) >= 0
>               && (ext & 5) == 4 ) /* SMSW / LMSW */
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
> index e88245eae9..d6c04fd5f3 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1479,15 +1479,15 @@ x86_emulate(
>          break;
>      }
>  
> +    enum x86_segment seg;
> +    struct segment_register cs, sreg;
> +    struct cpuid_leaf leaf;
> +    uint64_t msr_val;
> +    unsigned int i, n;
> +    unsigned long dummy;
> +
>      switch ( ctxt->opcode )
>      {
> -        enum x86_segment seg;
> -        struct segment_register cs, sreg;
> -        struct cpuid_leaf leaf;
> -        uint64_t msr_val;
> -        unsigned int i, n;
> -        unsigned long dummy;

In Xen we don't mix declarations and code, so they would have to go to
the top of the function


>      case 0x00: case 0x01: add: /* add reg,mem */
>          if ( ops->rmw && dst.type == OP_MEM )
>              state->rmw = rmw_add;
> -- 
> 2.34.1
> 

Re: [XEN PATCH 05/11] drivers/pci: move variable definitions to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> Variable declarations between a switch statement guard and before
> any case label are unreachable code, and hence violate Rule 2.1:
> "A project shall not contain unreachable code".
> 
> Therefore the variable declarations are moved in the smallest enclosing
> scope, near other variable definitions.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/drivers/passthrough/pci.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 33452791a8..3f5fa5beef 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -315,6 +315,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>      struct pci_dev *pdev;
>      unsigned int pos;
>      int rc;
> +    unsigned int cap, sec_bus, sub_bus;
>  
>      list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>          if ( pdev->bus == bus && pdev->devfn == devfn )
> @@ -343,8 +344,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>      /* update bus2bridge */
>      switch ( pdev->type = pdev_type(pseg->nr, bus, devfn) )
>      {
> -        unsigned int cap, sec_bus, sub_bus;

sub_bus and sec_bus could go under case DEV_TYPE_LEGACY_PCI_BRIDGE
cap could go under case DEV_TYPE_PCIe_ENDPOINT


>          case DEV_TYPE_PCIe2PCI_BRIDGE:
>          case DEV_TYPE_LEGACY_PCI_BRIDGE:
>              sec_bus = pci_conf_read8(pdev->sbdf, PCI_SECONDARY_BUS);
> @@ -424,11 +423,11 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>  
>  static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>  {
> +    unsigned int sec_bus, sub_bus;
> +
>      /* update bus2bridge */
>      switch ( pdev->type )
>      {
> -        unsigned int sec_bus, sub_bus;

They could go under case DEV_TYPE_LEGACY_PCI_BRIDGE


>          case DEV_TYPE_PCIe2PCI_BRIDGE:
>          case DEV_TYPE_LEGACY_PCI_BRIDGE:
>              sec_bus = pci_conf_read8(pdev->sbdf, PCI_SECONDARY_BUS);
> @@ -1555,11 +1554,10 @@ int iommu_do_pci_domctl(
>      u8 bus, devfn;
>      int ret = 0;
>      uint32_t machine_sbdf;
> +    unsigned int flags;
>  
>      switch ( domctl->cmd )
>      {
> -        unsigned int flags;

it could go under case XEN_DOMCTL_test_assign_device


>      case XEN_DOMCTL_get_device_group:
>      {
>          u32 max_sdevs;
> -- 
> 2.34.1
> 

Re: [XEN PATCH 01/11] x86/efi: move variable declaration to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 03.08.2023 04:08, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> The variable declaration is moved where it's actually used, rather
>> than being declared in the switch before any clause, thus being
>> classified as unreachable code.
>>
>> No functional changes.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/arch/x86/efi/efi-boot.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index 92f4cfe8bd..b00441b1a2 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -390,8 +390,6 @@ static void __init efi_arch_edd(void)
>>          {
>>              switch ( DevicePathType(devp.DevPath) )
>>              {
>> -                const u8 *p;
>> -
>>              case ACPI_DEVICE_PATH:
>>                  if ( state != root || boot_edd_info_nr > EDD_INFO_MAX )
>>                      break;
>> @@ -463,7 +461,8 @@ static void __init efi_arch_edd(void)
>>                  params->device_path_info_length =
>>                      sizeof(struct edd_device_params) -
>>                      offsetof(struct edd_device_params, key);
>> -                for ( p = (const u8 *)&params->key; p < &params->checksum; ++p )
>> +                for ( const u8 *p = (const u8 *)&params->key;
>> +                      p < &params->checksum; ++p )
> 
> In Xen we don't mix declaration and code. So the following is not
> something we use:
> 
>   for (int i = 0; i < 10; i++)

You're aware that we gained a couple of such uses already? I also think
that when we discussed this we said this style could be at least
okay-ish (until formalized in ./CODING_STYLE).

What I'm unhappy with here is the retaining of u8, when it could easily
become uint8_t at this occasion.

Jan

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 03.08.2023 04:13, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>>  
>>      switch ( action->ack_type )
>>      {
>> -        cpumask_t *cpu_eoi_map;
> 
> It is only used by case ACKTYPE_EOI so it can be moved there (with a new
> block):
> 
> 
>     case ACKTYPE_EOI:
>     {
>         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
>         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
>         spin_unlock_irq(&desc->lock);
>         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
>         return;
>     }
>     }

This pattern (two closing braces at the same level) is why switch scope
variable declarations were introduced (at least as far as introductions
by me go). If switch scope variables aren't okay (which I continue to
consider questionable), then this stylistic aspect needs sorting first
(if everyone else thinks the above style is okay - with the missing
blank line inserted -, then so be it).

Jan

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 02.08.2023 16:38, Nicola Vetrini wrote:
> Variable declarations between a switch statement guard and before
> any case label are unreachable code, and hence violate Rule 2.1:
> "A project shall not contain unreachable code".
> 
> Therefore the declarations are moved in the smallest enclosing
> scope, near other variable definitions.

This needs further clarification: There's no "code" involved here
in the sense that a compiler might eliminate it. Yet that's what
the rule's rationale is focusing on. There's no even mention of
declarations like the ones you move. In particular (and on top
of my comment on patch 1) ...

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -37,6 +37,7 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>  {
>      const struct domain *d = v->domain;
>      const struct cpu_policy *p = d->arch.cpu_policy;
> +    const struct cpu_user_regs *regs;
>  
>      *res = EMPTY_LEAF;
>  
> @@ -136,8 +137,6 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>       */
>      switch ( leaf )
>      {
> -        const struct cpu_user_regs *regs;
> -
>      case 0x1:
>          /* TODO: Rework topology logic. */
>          res->b &= 0x00ffffffu;

... I'm not happy to see scopes of variables widened.

Jan

Re: [XEN PATCH 04/11] x86emul: move variable definitions to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 03.08.2023 04:33, Stefano Stabellini wrote:
> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>> @@ -2289,16 +2289,15 @@ static int cf_check hvmemul_cache_op(
>>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>      uint32_t pfec = PFEC_page_present;
>> +    unsigned long addr;
>> +    int rc;
>> +    void *mapping;
>>  
>>      if ( !cache_flush_permitted(current->domain) )
>>          return X86EMUL_OKAY;
>>  
>>      switch ( op )
>>      {
>> -        unsigned long addr;
>> -        int rc;
>> -        void *mapping;
> 
> These three could be...
> 
> 
>>      case x86emul_clflush:
>>      case x86emul_clflushopt:
>>      case x86emul_clwb:
> 
> ... here in a new block

Except they're likely to be re-used as new enumerators are added.

>> --- a/xen/arch/x86/x86_emulate/util-xen.c
>> +++ b/xen/arch/x86/x86_emulate/util-xen.c
>> @@ -77,10 +77,10 @@ bool cf_check x86_insn_is_portio(const struct x86_emulate_state *s,
>>  bool cf_check x86_insn_is_cr_access(const struct x86_emulate_state *s,
>>                                      const struct x86_emulate_ctxt *ctxt)
>>  {
>> +    unsigned int ext;
>> +
>>      switch ( ctxt->opcode )
>>      {
>> -        unsigned int ext;
> 
> This can go under case X86EMUL_OPC with a new block

Same here.

Jan

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 02.08.2023 16:38, Nicola Vetrini wrote:
> Rule 2.1 states: "A project shall not contain unreachable code".
> 
> The functions
> - machine_halt
> - maybe_reboot
> - machine_restart
> are not supposed to return, hence the following break statement
> is marked as intentionally unreachable with the ASSERT_UNREACHABLE()
> macro to justify the violation of the rule.

During the discussion it was mentioned that this won't help with
release builds, where right now ASSERT_UNREACHABLE() expands to
effectively nothing. You want to clarify here how release builds
are to be taken care of, as those are what eventual certification
will be run against.

Jan

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 02.08.2023 16:38, Nicola Vetrini wrote:
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
>          /* fallthrough */
>      case TASKLET_enqueued|TASKLET_scheduled:
>          return true;
> +        ASSERT_UNREACHABLE();
>          break;

What use is "break" after "return"? IOW rather than adding code here,
imo a line wants removing.

Jan

Re: [XEN PATCH 11/11] x86/mm: Add assertion to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 02.08.2023 16:38, Nicola Vetrini wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return subarch_memory_op(cmd, arg);
>      }
>  
> +    ASSERT_UNREACHABLE();
>      return 0;
>  }

I'd prefer to instead switch earlier "return 0" to "break".

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const gfn_t *gfns, unsigned int count
>      }
>  
>      return;
> +    ASSERT_UNREACHABLE();
>  
>  out_unmap:
>      /*

In the description you say "before", but here you add something _after_
"return". What's the deal?

Jan

Re: [XEN PATCH 11/11] x86/mm: Add assertion to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 03/08/2023 11:20, Jan Beulich wrote:
> On 02.08.2023 16:38, Nicola Vetrini wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4879,6 +4879,7 @@ long arch_memory_op(unsigned long cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>          return subarch_memory_op(cmd, arg);
>>      }
>> 
>> +    ASSERT_UNREACHABLE();
>>      return 0;
>>  }
> 
> I'd prefer to instead switch earlier "return 0" to "break".

Ok

> 
>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const 
>> gfn_t *gfns, unsigned int count
>>      }
>> 
>>      return;
>> +    ASSERT_UNREACHABLE();
>> 
>>  out_unmap:
>>      /*
> 
> In the description you say "before", but here you add something _after_
> "return". What's the deal?
> 
> Jan

In this case the unreachable part is that after the label (looking at it 
now, I should have
put the assert after the label to make it clear), because earlier all 
jumps to
'out_unmap' are like this:

   ASSERT_UNREACHABLE();
   domain_crash(d);
   goto out_unmap;

As I understood it, this is a defensive coding measure, preventing pages 
to remain mapped if,
for some reason the above code actually executes. Am I correct?

Regards,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 03/08/2023 11:01, Jan Beulich wrote:
> On 03.08.2023 04:13, Stefano Stabellini wrote:
>> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>>> @@ -1169,8 +1170,6 @@ static void cf_check 
>>> irq_guest_eoi_timer_fn(void *data)
>>> 
>>>      switch ( action->ack_type )
>>>      {
>>> -        cpumask_t *cpu_eoi_map;
>> 
>> It is only used by case ACKTYPE_EOI so it can be moved there (with a 
>> new
>> block):
>> 
>> 
>>     case ACKTYPE_EOI:
>>     {
>>         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
>>         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
>>         spin_unlock_irq(&desc->lock);
>>         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
>>         return;
>>     }
>>     }
> 
> This pattern (two closing braces at the same level) is why switch scope
> variable declarations were introduced (at least as far as introductions
> by me go). If switch scope variables aren't okay (which I continue to
> consider questionable), then this stylistic aspect needs sorting first
> (if everyone else thinks the above style is okay - with the missing
> blank line inserted -, then so be it).
> 
> Jan

Actually, they can be deviated because they don't result in wrong code 
being generated.
This, modulo the review comments received, is what most of the code 
would look like if
they weren't, with the biggest pain point being that in many cases the 
choice is either
the pattern with blocks for certain clauses or moving them in the 
enclosing scope, which may
be several hundred lines above. If there's agreement on deviating them, 
I can drop the patches
dealing with switches and do a v2 with just the other modifications.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 11/11] x86/mm: Add assertion to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 03.08.2023 11:30, Nicola Vetrini wrote:
> On 03/08/2023 11:20, Jan Beulich wrote:
>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -1045,6 +1045,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, const 
>>> gfn_t *gfns, unsigned int count
>>>      }
>>>
>>>      return;
>>> +    ASSERT_UNREACHABLE();
>>>
>>>  out_unmap:
>>>      /*
>>
>> In the description you say "before", but here you add something _after_
>> "return". What's the deal?
> 
> In this case the unreachable part is that after the label (looking at it 
> now, I should have
> put the assert after the label to make it clear), because earlier all 
> jumps to
> 'out_unmap' are like this:
> 
>    ASSERT_UNREACHABLE();
>    domain_crash(d);
>    goto out_unmap;
> 
> As I understood it, this is a defensive coding measure, preventing pages 
> to remain mapped if,
> for some reason the above code actually executes. Am I correct?

The comment there says "probably". So the label and following code might
be used for another error condition as well. Furthermore both paths
presently using "goto out_unmap" already have ASSERT_UNREACHABLE(), so
it's hard to see why we would need yet one more.

Jan

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Thu, 3 Aug 2023, Nicola Vetrini wrote:
> On 03/08/2023 11:01, Jan Beulich wrote:
> > On 03.08.2023 04:13, Stefano Stabellini wrote:
> > > On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> > > > @@ -1169,8 +1170,6 @@ static void cf_check irq_guest_eoi_timer_fn(void
> > > > *data)
> > > > 
> > > >      switch ( action->ack_type )
> > > >      {
> > > > -        cpumask_t *cpu_eoi_map;
> > > 
> > > It is only used by case ACKTYPE_EOI so it can be moved there (with a new
> > > block):
> > > 
> > > 
> > >     case ACKTYPE_EOI:
> > >     {
> > >         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
> > >         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
> > >         spin_unlock_irq(&desc->lock);
> > >         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
> > >         return;
> > >     }
> > >     }
> > 
> > This pattern (two closing braces at the same level) is why switch scope
> > variable declarations were introduced (at least as far as introductions
> > by me go). If switch scope variables aren't okay (which I continue to
> > consider questionable), then this stylistic aspect needs sorting first
> > (if everyone else thinks the above style is okay - with the missing
> > blank line inserted -, then so be it).
> > 
> > Jan
> 
> Actually, they can be deviated because they don't result in wrong code being
> generated.
> This, modulo the review comments received, is what most of the code would look
> like if
> they weren't, with the biggest pain point being that in many cases the choice
> is either
> the pattern with blocks for certain clauses or moving them in the enclosing
> scope, which may
> be several hundred lines above. If there's agreement on deviating them, I can
> drop the patches
> dealing with switches and do a v2 with just the other modifications.

I think we should deviate them. Good idea to remove them in v2.

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Thu, 3 Aug 2023, Jan Beulich wrote:
> On 02.08.2023 16:38, Nicola Vetrini wrote:
> > Rule 2.1 states: "A project shall not contain unreachable code".
> > 
> > The functions
> > - machine_halt
> > - maybe_reboot
> > - machine_restart
> > are not supposed to return, hence the following break statement
> > is marked as intentionally unreachable with the ASSERT_UNREACHABLE()
> > macro to justify the violation of the rule.
> 
> During the discussion it was mentioned that this won't help with
> release builds, where right now ASSERT_UNREACHABLE() expands to
> effectively nothing. You want to clarify here how release builds
> are to be taken care of, as those are what eventual certification
> will be run against.

Something along these lines:

ASSERT_UNREACHABLE(), not only is used in non-release builds to actually
assert and detect errors, but it is also used as a marker to tag
unreachable code. In release builds ASSERT_UNREACHABLE() doesn't resolve
into an assert, but retains its role of a code marker.

Does it work?

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Thu, 3 Aug 2023, Stefano Stabellini wrote:
> > Actually, they can be deviated because they don't result in wrong code being
> > generated.
> > This, modulo the review comments received, is what most of the code would look
> > like if
> > they weren't, with the biggest pain point being that in many cases the choice
> > is either
> > the pattern with blocks for certain clauses or moving them in the enclosing
> > scope, which may
> > be several hundred lines above. If there's agreement on deviating them, I can
> > drop the patches
> > dealing with switches and do a v2 with just the other modifications.
> 
> I think we should deviate them. Good idea to remove them in v2.

We should add a note about this to docs/misra/rules.rst as well?

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 8f0e4d3f25..e713b0ea99 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -106,7 +106,8 @@ maintainers if you want to suggest a change.
    * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_
      - Required
      - A project shall not contain unreachable code
-     -
+     - It is acceptable to declare local variables under a switch
+       statement block
 
    * - `Rule 2.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_06.c>`_
      - Advisory

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 04.08.2023 01:50, Stefano Stabellini wrote:
> On Thu, 3 Aug 2023, Jan Beulich wrote:
>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>
>>> The functions
>>> - machine_halt
>>> - maybe_reboot
>>> - machine_restart
>>> are not supposed to return, hence the following break statement
>>> is marked as intentionally unreachable with the ASSERT_UNREACHABLE()
>>> macro to justify the violation of the rule.
>>
>> During the discussion it was mentioned that this won't help with
>> release builds, where right now ASSERT_UNREACHABLE() expands to
>> effectively nothing. You want to clarify here how release builds
>> are to be taken care of, as those are what eventual certification
>> will be run against.
> 
> Something along these lines:
> 
> ASSERT_UNREACHABLE(), not only is used in non-release builds to actually
> assert and detect errors, but it is also used as a marker to tag
> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't resolve
> into an assert, but retains its role of a code marker.
> 
> Does it work?

Well, it states what is happening, but I'm not convinced it satisfies
rule 2.1. There's then still code there which isn't reachable, and
which a scanner will spot and report.

Jan

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 03.08.2023 16:22, Nicola Vetrini wrote:
> On 03/08/2023 11:01, Jan Beulich wrote:
>> On 03.08.2023 04:13, Stefano Stabellini wrote:
>>> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>>>> @@ -1169,8 +1170,6 @@ static void cf_check 
>>>> irq_guest_eoi_timer_fn(void *data)
>>>>
>>>>      switch ( action->ack_type )
>>>>      {
>>>> -        cpumask_t *cpu_eoi_map;
>>>
>>> It is only used by case ACKTYPE_EOI so it can be moved there (with a 
>>> new
>>> block):
>>>
>>>
>>>     case ACKTYPE_EOI:
>>>     {
>>>         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
>>>         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
>>>         spin_unlock_irq(&desc->lock);
>>>         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
>>>         return;
>>>     }
>>>     }
>>
>> This pattern (two closing braces at the same level) is why switch scope
>> variable declarations were introduced (at least as far as introductions
>> by me go). If switch scope variables aren't okay (which I continue to
>> consider questionable), then this stylistic aspect needs sorting first
>> (if everyone else thinks the above style is okay - with the missing
>> blank line inserted -, then so be it).
> 
> Actually, they can be deviated because they don't result in wrong code 
> being generated.

Only later I recalled Andrew's intention to possibly make use of
-ftrivial-auto-var-init. While, unlike I think he said, such declared
variables aren't getting in the way of this (neither gcc nor clang
warn about them), they also don't benefit from it (i.e. they'll be
left uninitialized despite the command line option saying otherwise).
IOW I think further consideration is going to be needed here.

Jan

Re: [XEN PATCH 01/11] x86/efi: move variable declaration to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 03/08/2023 10:57, Jan Beulich wrote:
> On 03.08.2023 04:08, Stefano Stabellini wrote:
>> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>>> The variable declaration is moved where it's actually used, rather
>>> than being declared in the switch before any clause, thus being
>>> classified as unreachable code.
>>> 
>>> No functional changes.
>>> 
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> ---
>>>  xen/arch/x86/efi/efi-boot.h | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/xen/arch/x86/efi/efi-boot.h 
>>> b/xen/arch/x86/efi/efi-boot.h
>>> index 92f4cfe8bd..b00441b1a2 100644
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -390,8 +390,6 @@ static void __init efi_arch_edd(void)
>>>          {
>>>              switch ( DevicePathType(devp.DevPath) )
>>>              {
>>> -                const u8 *p;
>>> -
>>>              case ACPI_DEVICE_PATH:
>>>                  if ( state != root || boot_edd_info_nr > 
>>> EDD_INFO_MAX )
>>>                      break;
>>> @@ -463,7 +461,8 @@ static void __init efi_arch_edd(void)
>>>                  params->device_path_info_length =
>>>                      sizeof(struct edd_device_params) -
>>>                      offsetof(struct edd_device_params, key);
>>> -                for ( p = (const u8 *)&params->key; p < 
>>> &params->checksum; ++p )
>>> +                for ( const u8 *p = (const u8 *)&params->key;
>>> +                      p < &params->checksum; ++p )
>> 
>> In Xen we don't mix declaration and code. So the following is not
>> something we use:
>> 
>>   for (int i = 0; i < 10; i++)
> 
> You're aware that we gained a couple of such uses already? I also think
> that when we discussed this we said this style could be at least
> okay-ish (until formalized in ./CODING_STYLE).
> 
> What I'm unhappy with here is the retaining of u8, when it could easily
> become uint8_t at this occasion.
> 
> Jan

Sure

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

>>> 
>>> This pattern (two closing braces at the same level) is why switch 
>>> scope
>>> variable declarations were introduced (at least as far as 
>>> introductions
>>> by me go). If switch scope variables aren't okay (which I continue to
>>> consider questionable), then this stylistic aspect needs sorting 
>>> first
>>> (if everyone else thinks the above style is okay - with the missing
>>> blank line inserted -, then so be it).
>> 
>> Actually, they can be deviated because they don't result in wrong code
>> being generated.
> 
> Only later I recalled Andrew's intention to possibly make use of
> -ftrivial-auto-var-init. While, unlike I think he said, such declared
> variables aren't getting in the way of this (neither gcc nor clang
> warn about them), they also don't benefit from it (i.e. they'll be
> left uninitialized despite the command line option saying otherwise).
> IOW I think further consideration is going to be needed here.
> 
> Jan

Well, in that case I'm happy if I can contribute in some way to the 
discussion, but
perhaps this is best sorted out in a separate discussion, so that later 
I can send a v2
reflecting the decision(s).

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Fri, 4 Aug 2023, Jan Beulich wrote:
> On 03.08.2023 16:22, Nicola Vetrini wrote:
> > On 03/08/2023 11:01, Jan Beulich wrote:
> >> On 03.08.2023 04:13, Stefano Stabellini wrote:
> >>> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
> >>>> @@ -1169,8 +1170,6 @@ static void cf_check 
> >>>> irq_guest_eoi_timer_fn(void *data)
> >>>>
> >>>>      switch ( action->ack_type )
> >>>>      {
> >>>> -        cpumask_t *cpu_eoi_map;
> >>>
> >>> It is only used by case ACKTYPE_EOI so it can be moved there (with a 
> >>> new
> >>> block):
> >>>
> >>>
> >>>     case ACKTYPE_EOI:
> >>>     {
> >>>         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
> >>>         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
> >>>         spin_unlock_irq(&desc->lock);
> >>>         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
> >>>         return;
> >>>     }
> >>>     }
> >>
> >> This pattern (two closing braces at the same level) is why switch scope
> >> variable declarations were introduced (at least as far as introductions
> >> by me go). If switch scope variables aren't okay (which I continue to
> >> consider questionable), then this stylistic aspect needs sorting first
> >> (if everyone else thinks the above style is okay - with the missing
> >> blank line inserted -, then so be it).
> > 
> > Actually, they can be deviated because they don't result in wrong code 
> > being generated.
> 
> Only later I recalled Andrew's intention to possibly make use of
> -ftrivial-auto-var-init. While, unlike I think he said, such declared
> variables aren't getting in the way of this (neither gcc nor clang
> warn about them), they also don't benefit from it (i.e. they'll be
> left uninitialized despite the command line option saying otherwise).
> IOW I think further consideration is going to be needed here.

Let me get this right. Are you saying that if we enable
-ftrivial-auto-var-init, due to a compiler limitation, variables
declared as follow:

  switch(var) {
      int a;
      char b;
      
      case ...

do not benefit from -ftrivial-auto-var-init ?

So if we moved the variable declarations elsewhere, in one of the two
following ways:

1)
  int a;
  int b;

  switch(var) {

2)
  switch(var) {

  case 1:
  {
      int a;
      int b;


Then -ftrivial-auto-var-init would take effect? If so, I think it is
worth a discussion on what to do there.

But either way right now I think it is fine to take the switch-related
patches out of this series.

Re: [XEN PATCH 02/11] x86: move declarations to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 04.08.2023 22:26, Stefano Stabellini wrote:
> On Fri, 4 Aug 2023, Jan Beulich wrote:
>> On 03.08.2023 16:22, Nicola Vetrini wrote:
>>> On 03/08/2023 11:01, Jan Beulich wrote:
>>>> On 03.08.2023 04:13, Stefano Stabellini wrote:
>>>>> On Wed, 2 Aug 2023, Nicola Vetrini wrote:
>>>>>> @@ -1169,8 +1170,6 @@ static void cf_check 
>>>>>> irq_guest_eoi_timer_fn(void *data)
>>>>>>
>>>>>>      switch ( action->ack_type )
>>>>>>      {
>>>>>> -        cpumask_t *cpu_eoi_map;
>>>>>
>>>>> It is only used by case ACKTYPE_EOI so it can be moved there (with a 
>>>>> new
>>>>> block):
>>>>>
>>>>>
>>>>>     case ACKTYPE_EOI:
>>>>>     {
>>>>>         cpumask_t *cpu_eoi_map = this_cpu(scratch_cpumask);
>>>>>         cpumask_copy(cpu_eoi_map, action->cpu_eoi_map);
>>>>>         spin_unlock_irq(&desc->lock);
>>>>>         on_selected_cpus(cpu_eoi_map, set_eoi_ready, desc, 0);
>>>>>         return;
>>>>>     }
>>>>>     }
>>>>
>>>> This pattern (two closing braces at the same level) is why switch scope
>>>> variable declarations were introduced (at least as far as introductions
>>>> by me go). If switch scope variables aren't okay (which I continue to
>>>> consider questionable), then this stylistic aspect needs sorting first
>>>> (if everyone else thinks the above style is okay - with the missing
>>>> blank line inserted -, then so be it).
>>>
>>> Actually, they can be deviated because they don't result in wrong code 
>>> being generated.
>>
>> Only later I recalled Andrew's intention to possibly make use of
>> -ftrivial-auto-var-init. While, unlike I think he said, such declared
>> variables aren't getting in the way of this (neither gcc nor clang
>> warn about them), they also don't benefit from it (i.e. they'll be
>> left uninitialized despite the command line option saying otherwise).
>> IOW I think further consideration is going to be needed here.
> 
> Let me get this right. Are you saying that if we enable
> -ftrivial-auto-var-init, due to a compiler limitation, variables
> declared as follow:
> 
>   switch(var) {
>       int a;
>       char b;
>       
>       case ...
> 
> do not benefit from -ftrivial-auto-var-init ?

Yes, that's my observation with both compilers.

Jan

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 03/08/2023 11:17, Jan Beulich wrote:
> On 02.08.2023 16:38, Nicola Vetrini wrote:
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int 
>> cpu)
>>          /* fallthrough */
>>      case TASKLET_enqueued|TASKLET_scheduled:
>>          return true;
>> +        ASSERT_UNREACHABLE();
>>          break;
> 
> What use is "break" after "return"? IOW rather than adding code here,
> imo a line wants removing.
> 
> Jan

The "return false" after the switch would still be unreachable. The 
reasoning behind preserving the break
is mainly MISRA Rule 16.3: "An unconditional break statement shall 
terminate every switch-clause", which has
not yet been considered for adoption, but might be in future 
discussions, leading to putting back the break here.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 07.08.2023 10:13, Nicola Vetrini wrote:
> On 03/08/2023 11:17, Jan Beulich wrote:
>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int 
>>> cpu)
>>>          /* fallthrough */
>>>      case TASKLET_enqueued|TASKLET_scheduled:
>>>          return true;
>>> +        ASSERT_UNREACHABLE();
>>>          break;
>>
>> What use is "break" after "return"? IOW rather than adding code here,
>> imo a line wants removing.
> 
> The "return false" after the switch would still be unreachable. The 
> reasoning behind preserving the break
> is mainly MISRA Rule 16.3: "An unconditional break statement shall 
> terminate every switch-clause", which has
> not yet been considered for adoption, but might be in future 
> discussions, leading to putting back the break here.

Well, adding such bogus "break"s is going to face my opposition, and
it is in direct conflict with the "no unreachable code" rule here.

Jan

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 04/08/2023 08:42, Jan Beulich wrote:
> On 04.08.2023 01:50, Stefano Stabellini wrote:
>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>> 
>>>> The functions
>>>> - machine_halt
>>>> - maybe_reboot
>>>> - machine_restart
>>>> are not supposed to return, hence the following break statement
>>>> is marked as intentionally unreachable with the ASSERT_UNREACHABLE()
>>>> macro to justify the violation of the rule.
>>> 
>>> During the discussion it was mentioned that this won't help with
>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>> effectively nothing. You want to clarify here how release builds
>>> are to be taken care of, as those are what eventual certification
>>> will be run against.
>> 
>> Something along these lines:
>> 
>> ASSERT_UNREACHABLE(), not only is used in non-release builds to 
>> actually
>> assert and detect errors, but it is also used as a marker to tag
>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't 
>> resolve
>> into an assert, but retains its role of a code marker.
>> 
>> Does it work?
> 
> Well, it states what is happening, but I'm not convinced it satisfies
> rule 2.1. There's then still code there which isn't reachable, and
> which a scanner will spot and report.
> 
> Jan

It's not clear to me whether you dislike the patch itself or the commit
message. If it's the latter, how about:
"ASSERT_UNREACHABLE() is used as a marker for intentionally unreachable 
code, which
constitutes a motivated deviation from Rule 2.1. Additionally, in 
non-release
builds, this macro performs a failing assertion to detect errors."

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Julien Grall]

Hi,

On 02/08/2023 15:38, Nicola Vetrini wrote:
> The break statement after the return statement is definitely unreachable.
> As such, an call to the ASSERT_UNREACHABLE() macro is added to signal
> the intentionality of such construct.

How about using unreachable() rather than ASSERT_UNREACHABLE()? The main 
difference is the later will hint the compiler that the code cannot be 
reached and will not crash Xen in debug build (this could be changed).

> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> The break in the clause is mandated by Required Rule 16.3, which is
> not yet an accepted rule for Xen, but may be in the future.
> ---
>   xen/common/sched/core.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 022f548652..fcee902b4e 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
>           /* fallthrough */
>       case TASKLET_enqueued|TASKLET_scheduled:
>           return true;
> +        ASSERT_UNREACHABLE();
>           break;
>       case TASKLET_scheduled:
>           clear_bit(_TASKLET_scheduled, tasklet_work);

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 08.08.2023 17:25, Julien Grall wrote:
> On 02/08/2023 15:38, Nicola Vetrini wrote:
>> The break statement after the return statement is definitely unreachable.
>> As such, an call to the ASSERT_UNREACHABLE() macro is added to signal
>> the intentionality of such construct.
> 
> How about using unreachable() rather than ASSERT_UNREACHABLE()? The main 
> difference is the later will hint the compiler that the code cannot be 
> reached and will not crash Xen in debug build (this could be changed).

Isn't using unreachable() in place of ASSERT_UNREACHABLE() unsafe (not
here but in general)? It'll tell the compiler that (in the extreme case)
it can omit the function epilogue, including the return instruction. In
the resulting build, if the code turns out to be reachable, execution
would fall through into whatever follows.

In the case here ...

>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
>>           /* fallthrough */
>>       case TASKLET_enqueued|TASKLET_scheduled:
>>           return true;
>> +        ASSERT_UNREACHABLE();
>>           break;
>>       case TASKLET_scheduled:
>>           clear_bit(_TASKLET_scheduled, tasklet_work);

... "return" alone already tells the compiler that "break" is
unreachable. You don't need unreachable() for that. As said before,
"break" like this simply want purging (and Misra is wrong to demand
"break" everywhere - it should instead demand no [unannotated]
fall-through, which can also be achieved by return, continue, and
goto).

Jan

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Julien Grall]

Hi,

On 08/08/2023 16:36, Jan Beulich wrote:
> On 08.08.2023 17:25, Julien Grall wrote:
>> On 02/08/2023 15:38, Nicola Vetrini wrote:
>>> The break statement after the return statement is definitely unreachable.
>>> As such, an call to the ASSERT_UNREACHABLE() macro is added to signal
>>> the intentionality of such construct.
>>
>> How about using unreachable() rather than ASSERT_UNREACHABLE()? The main
>> difference is the later will hint the compiler that the code cannot be
>> reached and will not crash Xen in debug build (this could be changed).
> 
> Isn't using unreachable() in place of ASSERT_UNREACHABLE() unsafe (not
> here but in general)?

Yes it is.

  It'll tell the compiler that (in the extreme case)
> it can omit the function epilogue, including the return instruction. In
> the resulting build, if the code turns out to be reachable, execution
> would fall through into whatever follows.

Right, but the problem is somewhat similar with adding 
ASSERT_UNREACHABLE() without proper error path because there is no 
guarantee the rest of the code will be correct in the unlikely case it 
is reachable.

> 
> In the case here ...
> 
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -2233,6 +2233,7 @@ static bool sched_tasklet_check_cpu(unsigned int cpu)
>>>            /* fallthrough */
>>>        case TASKLET_enqueued|TASKLET_scheduled:
>>>            return true;
>>> +        ASSERT_UNREACHABLE();
>>>            break;
>>>        case TASKLET_scheduled:
>>>            clear_bit(_TASKLET_scheduled, tasklet_work);
> 
> ... "return" alone already tells the compiler that "break" is
> unreachable. You don't need unreachable() for that. As said before,
> "break" like this simply want purging (and Misra is wrong to demand
> "break" everywhere - it should instead demand no [unannotated]
> fall-through, which can also be achieved by return, continue, and
> goto).

My suggestion was in the context of this series where we add 
ASSERT_UNREACHABLE() before break. From my understanding, we don't have 
a lot of leeway here because, from what Nicola wrote, rule 16.3 is 
mandatory.

So I think using unreachable is better if we every decide to use break 
after return.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

>> 
>> ... "return" alone already tells the compiler that "break" is
>> unreachable. You don't need unreachable() for that. As said before,
>> "break" like this simply want purging (and Misra is wrong to demand
>> "break" everywhere - it should instead demand no [unannotated]
>> fall-through, which can also be achieved by return, continue, and
>> goto).
> 
> My suggestion was in the context of this series where we add
> ASSERT_UNREACHABLE() before break. From my understanding, we don't
> have a lot of leeway here because, from what Nicola wrote, rule 16.3
> is mandatory.
> 
> So I think using unreachable is better if we every decide to use break
> after return.
> 
> Cheers,

16.3 is not Mandatory, just Required. I was just reluctant to knowingly 
add one more violation
while fixing another before submitting the patch. If indeed 16.3 won't 
likely be adopted by Xen,
then that break should indeed go away.

However, the main thing that's holding me back from doing a slimmed-down 
v2 here is that
evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Julien Grall]

On 08/08/2023 16:53, Nicola Vetrini wrote:
> 
>>>
>>> ... "return" alone already tells the compiler that "break" is
>>> unreachable. You don't need unreachable() for that. As said before,
>>> "break" like this simply want purging (and Misra is wrong to demand
>>> "break" everywhere - it should instead demand no [unannotated]
>>> fall-through, which can also be achieved by return, continue, and
>>> goto).
>>
>> My suggestion was in the context of this series where we add
>> ASSERT_UNREACHABLE() before break. From my understanding, we don't
>> have a lot of leeway here because, from what Nicola wrote, rule 16.3
>> is mandatory.
>>
>> So I think using unreachable is better if we every decide to use break
>> after return.
>>
>> Cheers,
> 
> 16.3 is not Mandatory, just Required.

Ah I misread what you wrote. In that case...

> I was just reluctant to knowingly 
> add one more violation
> while fixing another before submitting the patch. If indeed 16.3 won't 
> likely be adopted by Xen,
> then that break should indeed go away.
> 
> However, the main thing that's holding me back from doing a slimmed-down 
> v2 here is that
> evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.

... I agree with the following statement from Jan:

"it should instead demand no [unannotated] fall-through, which can also 
be achieved by return, continue, and goto"

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Tue, 8 Aug 2023, Julien Grall wrote:
> On 08/08/2023 16:53, Nicola Vetrini wrote:
> > > > ... "return" alone already tells the compiler that "break" is
> > > > unreachable. You don't need unreachable() for that. As said before,
> > > > "break" like this simply want purging (and Misra is wrong to demand
> > > > "break" everywhere - it should instead demand no [unannotated]
> > > > fall-through, which can also be achieved by return, continue, and
> > > > goto).
> > > 
> > > My suggestion was in the context of this series where we add
> > > ASSERT_UNREACHABLE() before break. From my understanding, we don't
> > > have a lot of leeway here because, from what Nicola wrote, rule 16.3
> > > is mandatory.
> > > 
> > > So I think using unreachable is better if we every decide to use break
> > > after return.
> > > 
> > > Cheers,
> > 
> > 16.3 is not Mandatory, just Required.
> 
> Ah I misread what you wrote. In that case...
> 
> > I was just reluctant to knowingly add one more violation
> > while fixing another before submitting the patch. If indeed 16.3 won't
> > likely be adopted by Xen,
> > then that break should indeed go away.
> > 
> > However, the main thing that's holding me back from doing a slimmed-down v2
> > here is that
> > evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.
> 
> ... I agree with the following statement from Jan:
> 
> "it should instead demand no [unannotated] fall-through, which can also be
> achieved by return, continue, and goto"

I also think it is a better idea to have no unannotated fall-through in
switch statements (unless we have a "case" with nothing underneath, so
two cases next to each other). In other words the following are all OK
in my view:

  case A:
    do();
    break;
  case B:
    do();
    return true;
  case Ca:
  case Cb:
    goto fail;
  case D:
    i++;
    continue;
  case E:
    do();
    /* fall-through */
  case F:
    do();
    break;

While the following is not OK:

  case A:
    do();
  case B:
    do();

This should be what we already do in Xen, although it is not documented
properly. Jan, Julien do you agree?

If so, could Eclair be configured to check for this pattern (or
similar)?

We must have one of the following:
- break, continue, return, goto
- /* fall-through */
- empty case body (case Ca/Cb)

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 08.08.2023 23:14, Stefano Stabellini wrote:
> On Tue, 8 Aug 2023, Julien Grall wrote:
>> On 08/08/2023 16:53, Nicola Vetrini wrote:
>>>>> ... "return" alone already tells the compiler that "break" is
>>>>> unreachable. You don't need unreachable() for that. As said before,
>>>>> "break" like this simply want purging (and Misra is wrong to demand
>>>>> "break" everywhere - it should instead demand no [unannotated]
>>>>> fall-through, which can also be achieved by return, continue, and
>>>>> goto).
>>>>
>>>> My suggestion was in the context of this series where we add
>>>> ASSERT_UNREACHABLE() before break. From my understanding, we don't
>>>> have a lot of leeway here because, from what Nicola wrote, rule 16.3
>>>> is mandatory.
>>>>
>>>> So I think using unreachable is better if we every decide to use break
>>>> after return.
>>>>
>>>> Cheers,
>>>
>>> 16.3 is not Mandatory, just Required.
>>
>> Ah I misread what you wrote. In that case...
>>
>>> I was just reluctant to knowingly add one more violation
>>> while fixing another before submitting the patch. If indeed 16.3 won't
>>> likely be adopted by Xen,
>>> then that break should indeed go away.
>>>
>>> However, the main thing that's holding me back from doing a slimmed-down v2
>>> here is that
>>> evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.
>>
>> ... I agree with the following statement from Jan:
>>
>> "it should instead demand no [unannotated] fall-through, which can also be
>> achieved by return, continue, and goto"
> 
> I also think it is a better idea to have no unannotated fall-through in
> switch statements (unless we have a "case" with nothing underneath, so
> two cases next to each other). In other words the following are all OK
> in my view:
> 
>   case A:
>     do();
>     break;
>   case B:
>     do();
>     return true;
>   case Ca:
>   case Cb:
>     goto fail;
>   case D:
>     i++;
>     continue;
>   case E:
>     do();
>     /* fall-through */
>   case F:
>     do();
>     break;
> 
> While the following is not OK:
> 
>   case A:
>     do();
>   case B:
>     do();
> 
> This should be what we already do in Xen, although it is not documented
> properly. Jan, Julien do you agree?

Yes.

> If so, could Eclair be configured to check for this pattern (or
> similar)?
> 
> We must have one of the following:
> - break, continue, return, goto
> - /* fall-through */

- fallthrough;

(the pseudo keyword that we have; see compiler.h for actually having the
documentation you're looking for, albeit missing "continue", and of
course not really expected to live [just] there)

Jan

> - empty case body (case Ca/Cb)

Re: [XEN PATCH 10/11] xen/sched: add ASSERT_UNREACHABLE() to address MISRA C:2012 Rule 2.1

[Julien Grall]

Hi Stefano,

On 08/08/2023 22:14, Stefano Stabellini wrote:
> On Tue, 8 Aug 2023, Julien Grall wrote:
>> On 08/08/2023 16:53, Nicola Vetrini wrote:
>>>>> ... "return" alone already tells the compiler that "break" is
>>>>> unreachable. You don't need unreachable() for that. As said before,
>>>>> "break" like this simply want purging (and Misra is wrong to demand
>>>>> "break" everywhere - it should instead demand no [unannotated]
>>>>> fall-through, which can also be achieved by return, continue, and
>>>>> goto).
>>>>
>>>> My suggestion was in the context of this series where we add
>>>> ASSERT_UNREACHABLE() before break. From my understanding, we don't
>>>> have a lot of leeway here because, from what Nicola wrote, rule 16.3
>>>> is mandatory.
>>>>
>>>> So I think using unreachable is better if we every decide to use break
>>>> after return.
>>>>
>>>> Cheers,
>>>
>>> 16.3 is not Mandatory, just Required.
>>
>> Ah I misread what you wrote. In that case...
>>
>>> I was just reluctant to knowingly add one more violation
>>> while fixing another before submitting the patch. If indeed 16.3 won't
>>> likely be adopted by Xen,
>>> then that break should indeed go away.
>>>
>>> However, the main thing that's holding me back from doing a slimmed-down v2
>>> here is that
>>> evidently there's no consensus even on the ASSERT_UNREACHABLE() patches.
>>
>> ... I agree with the following statement from Jan:
>>
>> "it should instead demand no [unannotated] fall-through, which can also be
>> achieved by return, continue, and goto"
> 
> I also think it is a better idea to have no unannotated fall-through in
> switch statements (unless we have a "case" with nothing underneath, so
> two cases next to each other). In other words the following are all OK
> in my view:
> 
>    case A:
>      do();
>      break;
>    case B:
>      do();
>      return true;
>    case Ca:
>    case Cb:
>      goto fail;
>    case D:
>      i++;
>      continue;
>    case E:
>      do();
>      /* fall-through */
>    case F:
>      do();
>      break;
> 
> While the following is not OK:
> 
>    case A:
>      do();
>    case B:
>      do();
> 
> This should be what we already do in Xen, although it is not documented
> properly. Jan, Julien do you agree?

I agree.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

Hi,

On 08/08/2023 11:03, Nicola Vetrini wrote:
> On 04/08/2023 08:42, Jan Beulich wrote:
>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>>> 
>>>>> The functions
>>>>> - machine_halt
>>>>> - maybe_reboot
>>>>> - machine_restart
>>>>> are not supposed to return, hence the following break statement
>>>>> is marked as intentionally unreachable with the 
>>>>> ASSERT_UNREACHABLE()
>>>>> macro to justify the violation of the rule.
>>>> 
>>>> During the discussion it was mentioned that this won't help with
>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>> effectively nothing. You want to clarify here how release builds
>>>> are to be taken care of, as those are what eventual certification
>>>> will be run against.
>>> 
>>> Something along these lines:
>>> 
>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to 
>>> actually
>>> assert and detect errors, but it is also used as a marker to tag
>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't 
>>> resolve
>>> into an assert, but retains its role of a code marker.
>>> 
>>> Does it work?
>> 
>> Well, it states what is happening, but I'm not convinced it satisfies
>> rule 2.1. There's then still code there which isn't reachable, and
>> which a scanner will spot and report.
>> 
>> Jan
> 
> It's not clear to me whether you dislike the patch itself or the commit
> message. If it's the latter, how about:
> "ASSERT_UNREACHABLE() is used as a marker for intentionally
> unreachable code, which
> constitutes a motivated deviation from Rule 2.1. Additionally, in 
> non-release
> builds, this macro performs a failing assertion to detect errors."

Any feedback on this (with one edit: s/a failing assertion/an 
assertion/)

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 16.08.2023 12:01, Nicola Vetrini wrote:
> Hi,
> 
> On 08/08/2023 11:03, Nicola Vetrini wrote:
>> On 04/08/2023 08:42, Jan Beulich wrote:
>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>>>>
>>>>>> The functions
>>>>>> - machine_halt
>>>>>> - maybe_reboot
>>>>>> - machine_restart
>>>>>> are not supposed to return, hence the following break statement
>>>>>> is marked as intentionally unreachable with the 
>>>>>> ASSERT_UNREACHABLE()
>>>>>> macro to justify the violation of the rule.
>>>>>
>>>>> During the discussion it was mentioned that this won't help with
>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>> effectively nothing. You want to clarify here how release builds
>>>>> are to be taken care of, as those are what eventual certification
>>>>> will be run against.
>>>>
>>>> Something along these lines:
>>>>
>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to 
>>>> actually
>>>> assert and detect errors, but it is also used as a marker to tag
>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't 
>>>> resolve
>>>> into an assert, but retains its role of a code marker.
>>>>
>>>> Does it work?
>>>
>>> Well, it states what is happening, but I'm not convinced it satisfies
>>> rule 2.1. There's then still code there which isn't reachable, and
>>> which a scanner will spot and report.
>>
>> It's not clear to me whether you dislike the patch itself or the commit
>> message. If it's the latter, how about:
>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>> unreachable code, which
>> constitutes a motivated deviation from Rule 2.1. Additionally, in 
>> non-release
>> builds, this macro performs a failing assertion to detect errors."
> 
> Any feedback on this (with one edit: s/a failing assertion/an 
> assertion/)

The patch here is kind of okay, but I'm afraid I view my earlier question
as not addressed: How will the proposed change prevent the scanner from
spotting issues here in release builds? Just stating in the description
that there's a deviation is not going to help that.

Jan

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 16/08/2023 12:31, Jan Beulich wrote:
> On 16.08.2023 12:01, Nicola Vetrini wrote:
>> Hi,
>> 
>> On 08/08/2023 11:03, Nicola Vetrini wrote:
>>> On 04/08/2023 08:42, Jan Beulich wrote:
>>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>>>>> 
>>>>>>> The functions
>>>>>>> - machine_halt
>>>>>>> - maybe_reboot
>>>>>>> - machine_restart
>>>>>>> are not supposed to return, hence the following break statement
>>>>>>> is marked as intentionally unreachable with the
>>>>>>> ASSERT_UNREACHABLE()
>>>>>>> macro to justify the violation of the rule.
>>>>>> 
>>>>>> During the discussion it was mentioned that this won't help with
>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>>> effectively nothing. You want to clarify here how release builds
>>>>>> are to be taken care of, as those are what eventual certification
>>>>>> will be run against.
>>>>> 
>>>>> Something along these lines:
>>>>> 
>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to
>>>>> actually
>>>>> assert and detect errors, but it is also used as a marker to tag
>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't
>>>>> resolve
>>>>> into an assert, but retains its role of a code marker.
>>>>> 
>>>>> Does it work?
>>>> 
>>>> Well, it states what is happening, but I'm not convinced it 
>>>> satisfies
>>>> rule 2.1. There's then still code there which isn't reachable, and
>>>> which a scanner will spot and report.
>>> 
>>> It's not clear to me whether you dislike the patch itself or the 
>>> commit
>>> message. If it's the latter, how about:
>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>>> unreachable code, which
>>> constitutes a motivated deviation from Rule 2.1. Additionally, in
>>> non-release
>>> builds, this macro performs a failing assertion to detect errors."
>> 
>> Any feedback on this (with one edit: s/a failing assertion/an
>> assertion/)
> 
> The patch here is kind of okay, but I'm afraid I view my earlier 
> question
> as not addressed: How will the proposed change prevent the scanner from
> spotting issues here in release builds? Just stating in the description
> that there's a deviation is not going to help that.
> 
> Jan

There is a deviation already in place. At the moment it just ignores 
anything below an unreachable ASSERT_UNREACHABLE(), no matter what that 
macro will expand to.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 16.08.2023 12:47, Nicola Vetrini wrote:
> On 16/08/2023 12:31, Jan Beulich wrote:
>> On 16.08.2023 12:01, Nicola Vetrini wrote:
>>> On 08/08/2023 11:03, Nicola Vetrini wrote:
>>>> On 04/08/2023 08:42, Jan Beulich wrote:
>>>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
>>>>>>>>
>>>>>>>> The functions
>>>>>>>> - machine_halt
>>>>>>>> - maybe_reboot
>>>>>>>> - machine_restart
>>>>>>>> are not supposed to return, hence the following break statement
>>>>>>>> is marked as intentionally unreachable with the
>>>>>>>> ASSERT_UNREACHABLE()
>>>>>>>> macro to justify the violation of the rule.
>>>>>>>
>>>>>>> During the discussion it was mentioned that this won't help with
>>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>>>> effectively nothing. You want to clarify here how release builds
>>>>>>> are to be taken care of, as those are what eventual certification
>>>>>>> will be run against.
>>>>>>
>>>>>> Something along these lines:
>>>>>>
>>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to
>>>>>> actually
>>>>>> assert and detect errors, but it is also used as a marker to tag
>>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't
>>>>>> resolve
>>>>>> into an assert, but retains its role of a code marker.
>>>>>>
>>>>>> Does it work?
>>>>>
>>>>> Well, it states what is happening, but I'm not convinced it 
>>>>> satisfies
>>>>> rule 2.1. There's then still code there which isn't reachable, and
>>>>> which a scanner will spot and report.
>>>>
>>>> It's not clear to me whether you dislike the patch itself or the 
>>>> commit
>>>> message. If it's the latter, how about:
>>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>>>> unreachable code, which
>>>> constitutes a motivated deviation from Rule 2.1. Additionally, in
>>>> non-release
>>>> builds, this macro performs a failing assertion to detect errors."
>>>
>>> Any feedback on this (with one edit: s/a failing assertion/an
>>> assertion/)
>>
>> The patch here is kind of okay, but I'm afraid I view my earlier 
>> question
>> as not addressed: How will the proposed change prevent the scanner from
>> spotting issues here in release builds? Just stating in the description
>> that there's a deviation is not going to help that.
> 
> There is a deviation already in place. At the moment it just ignores 
> anything below an unreachable ASSERT_UNREACHABLE(), no matter what that 
> macro will expand to.

What do you mean by "in place"? docs/misra/ has nothing, afaics. And
automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering
things out of reports, aiui. (Plus of course there should be a single
central place where all deviations are recorded, imo.)

Jan

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

On 16/08/2023 13:23, Jan Beulich wrote:
> On 16.08.2023 12:47, Nicola Vetrini wrote:
>> On 16/08/2023 12:31, Jan Beulich wrote:
>>> On 16.08.2023 12:01, Nicola Vetrini wrote:
>>>> On 08/08/2023 11:03, Nicola Vetrini wrote:
>>>>> On 04/08/2023 08:42, Jan Beulich wrote:
>>>>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>>>>> Rule 2.1 states: "A project shall not contain unreachable 
>>>>>>>>> code".
>>>>>>>>> 
>>>>>>>>> The functions
>>>>>>>>> - machine_halt
>>>>>>>>> - maybe_reboot
>>>>>>>>> - machine_restart
>>>>>>>>> are not supposed to return, hence the following break statement
>>>>>>>>> is marked as intentionally unreachable with the
>>>>>>>>> ASSERT_UNREACHABLE()
>>>>>>>>> macro to justify the violation of the rule.
>>>>>>>> 
>>>>>>>> During the discussion it was mentioned that this won't help with
>>>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>>>>> effectively nothing. You want to clarify here how release builds
>>>>>>>> are to be taken care of, as those are what eventual 
>>>>>>>> certification
>>>>>>>> will be run against.
>>>>>>> 
>>>>>>> Something along these lines:
>>>>>>> 
>>>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to
>>>>>>> actually
>>>>>>> assert and detect errors, but it is also used as a marker to tag
>>>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't
>>>>>>> resolve
>>>>>>> into an assert, but retains its role of a code marker.
>>>>>>> 
>>>>>>> Does it work?
>>>>>> 
>>>>>> Well, it states what is happening, but I'm not convinced it
>>>>>> satisfies
>>>>>> rule 2.1. There's then still code there which isn't reachable, and
>>>>>> which a scanner will spot and report.
>>>>> 
>>>>> It's not clear to me whether you dislike the patch itself or the
>>>>> commit
>>>>> message. If it's the latter, how about:
>>>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>>>>> unreachable code, which
>>>>> constitutes a motivated deviation from Rule 2.1. Additionally, in
>>>>> non-release
>>>>> builds, this macro performs a failing assertion to detect errors."
>>>> 
>>>> Any feedback on this (with one edit: s/a failing assertion/an
>>>> assertion/)
>>> 
>>> The patch here is kind of okay, but I'm afraid I view my earlier
>>> question
>>> as not addressed: How will the proposed change prevent the scanner 
>>> from
>>> spotting issues here in release builds? Just stating in the 
>>> description
>>> that there's a deviation is not going to help that.
>> 
>> There is a deviation already in place. At the moment it just ignores
>> anything below an unreachable ASSERT_UNREACHABLE(), no matter what 
>> that
>> macro will expand to.
> 
> What do you mean by "in place"? docs/misra/ has nothing, afaics. And
> automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering
> things out of reports, aiui. (Plus of course there should be a single
> central place where all deviations are recorded, imo.)

The second statement is not quite correct, as some of the configurations 
instruct the
checker how to behave.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Jan Beulich]

On 16.08.2023 15:43, Nicola Vetrini wrote:
> On 16/08/2023 13:23, Jan Beulich wrote:
>> On 16.08.2023 12:47, Nicola Vetrini wrote:
>>> On 16/08/2023 12:31, Jan Beulich wrote:
>>>> On 16.08.2023 12:01, Nicola Vetrini wrote:
>>>>> On 08/08/2023 11:03, Nicola Vetrini wrote:
>>>>>> On 04/08/2023 08:42, Jan Beulich wrote:
>>>>>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
>>>>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
>>>>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
>>>>>>>>>> Rule 2.1 states: "A project shall not contain unreachable 
>>>>>>>>>> code".
>>>>>>>>>>
>>>>>>>>>> The functions
>>>>>>>>>> - machine_halt
>>>>>>>>>> - maybe_reboot
>>>>>>>>>> - machine_restart
>>>>>>>>>> are not supposed to return, hence the following break statement
>>>>>>>>>> is marked as intentionally unreachable with the
>>>>>>>>>> ASSERT_UNREACHABLE()
>>>>>>>>>> macro to justify the violation of the rule.
>>>>>>>>>
>>>>>>>>> During the discussion it was mentioned that this won't help with
>>>>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
>>>>>>>>> effectively nothing. You want to clarify here how release builds
>>>>>>>>> are to be taken care of, as those are what eventual 
>>>>>>>>> certification
>>>>>>>>> will be run against.
>>>>>>>>
>>>>>>>> Something along these lines:
>>>>>>>>
>>>>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to
>>>>>>>> actually
>>>>>>>> assert and detect errors, but it is also used as a marker to tag
>>>>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't
>>>>>>>> resolve
>>>>>>>> into an assert, but retains its role of a code marker.
>>>>>>>>
>>>>>>>> Does it work?
>>>>>>>
>>>>>>> Well, it states what is happening, but I'm not convinced it
>>>>>>> satisfies
>>>>>>> rule 2.1. There's then still code there which isn't reachable, and
>>>>>>> which a scanner will spot and report.
>>>>>>
>>>>>> It's not clear to me whether you dislike the patch itself or the
>>>>>> commit
>>>>>> message. If it's the latter, how about:
>>>>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
>>>>>> unreachable code, which
>>>>>> constitutes a motivated deviation from Rule 2.1. Additionally, in
>>>>>> non-release
>>>>>> builds, this macro performs a failing assertion to detect errors."
>>>>>
>>>>> Any feedback on this (with one edit: s/a failing assertion/an
>>>>> assertion/)
>>>>
>>>> The patch here is kind of okay, but I'm afraid I view my earlier
>>>> question
>>>> as not addressed: How will the proposed change prevent the scanner 
>>>> from
>>>> spotting issues here in release builds? Just stating in the 
>>>> description
>>>> that there's a deviation is not going to help that.
>>>
>>> There is a deviation already in place. At the moment it just ignores
>>> anything below an unreachable ASSERT_UNREACHABLE(), no matter what 
>>> that
>>> macro will expand to.
>>
>> What do you mean by "in place"? docs/misra/ has nothing, afaics. And
>> automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering
>> things out of reports, aiui. (Plus of course there should be a single
>> central place where all deviations are recorded, imo.)
> 
> The second statement is not quite correct, as some of the configurations 
> instruct the
> checker how to behave.

Well, I was referring to the one setting relevant here, and I added "aiui"
to indicate that I may be misreading what that file specifies.

Jan

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Stefano Stabellini]

On Wed, 16 Aug 2023, Jan Beulich wrote:
> On 16.08.2023 12:47, Nicola Vetrini wrote:
> > On 16/08/2023 12:31, Jan Beulich wrote:
> >> On 16.08.2023 12:01, Nicola Vetrini wrote:
> >>> On 08/08/2023 11:03, Nicola Vetrini wrote:
> >>>> On 04/08/2023 08:42, Jan Beulich wrote:
> >>>>> On 04.08.2023 01:50, Stefano Stabellini wrote:
> >>>>>> On Thu, 3 Aug 2023, Jan Beulich wrote:
> >>>>>>> On 02.08.2023 16:38, Nicola Vetrini wrote:
> >>>>>>>> Rule 2.1 states: "A project shall not contain unreachable code".
> >>>>>>>>
> >>>>>>>> The functions
> >>>>>>>> - machine_halt
> >>>>>>>> - maybe_reboot
> >>>>>>>> - machine_restart
> >>>>>>>> are not supposed to return, hence the following break statement
> >>>>>>>> is marked as intentionally unreachable with the
> >>>>>>>> ASSERT_UNREACHABLE()
> >>>>>>>> macro to justify the violation of the rule.
> >>>>>>>
> >>>>>>> During the discussion it was mentioned that this won't help with
> >>>>>>> release builds, where right now ASSERT_UNREACHABLE() expands to
> >>>>>>> effectively nothing. You want to clarify here how release builds
> >>>>>>> are to be taken care of, as those are what eventual certification
> >>>>>>> will be run against.
> >>>>>>
> >>>>>> Something along these lines:
> >>>>>>
> >>>>>> ASSERT_UNREACHABLE(), not only is used in non-release builds to
> >>>>>> actually
> >>>>>> assert and detect errors, but it is also used as a marker to tag
> >>>>>> unreachable code. In release builds ASSERT_UNREACHABLE() doesn't
> >>>>>> resolve
> >>>>>> into an assert, but retains its role of a code marker.
> >>>>>>
> >>>>>> Does it work?
> >>>>>
> >>>>> Well, it states what is happening, but I'm not convinced it 
> >>>>> satisfies
> >>>>> rule 2.1. There's then still code there which isn't reachable, and
> >>>>> which a scanner will spot and report.
> >>>>
> >>>> It's not clear to me whether you dislike the patch itself or the 
> >>>> commit
> >>>> message. If it's the latter, how about:
> >>>> "ASSERT_UNREACHABLE() is used as a marker for intentionally
> >>>> unreachable code, which
> >>>> constitutes a motivated deviation from Rule 2.1. Additionally, in
> >>>> non-release
> >>>> builds, this macro performs a failing assertion to detect errors."
> >>>
> >>> Any feedback on this (with one edit: s/a failing assertion/an
> >>> assertion/)
> >>
> >> The patch here is kind of okay, but I'm afraid I view my earlier 
> >> question
> >> as not addressed: How will the proposed change prevent the scanner from
> >> spotting issues here in release builds? Just stating in the description
> >> that there's a deviation is not going to help that.
> > 
> > There is a deviation already in place. At the moment it just ignores 
> > anything below an unreachable ASSERT_UNREACHABLE(), no matter what that 
> > macro will expand to.
> 
> What do you mean by "in place"? docs/misra/ has nothing, afaics. And
> automation/eclair_analysis/ECLAIR/deviations.ecl is merely filtering
> things out of reports, aiui. (Plus of course there should be a single
> central place where all deviations are recorded, imo.)

Jan has a point: I think we should record all our deviations and unique
ways to interpret the rules under docs/misra. And the Eclair
configuration should reflect that. It is not a good idea to only keep
the information in the Eclair config because, even if it is now upstream
in xen.git, it is still a tool-specific configuration file -- it is not
a proper document. MISRA C and its interpretation is important enough
that we should have a plain English document to cover it (which now is
docs/misra/rules.rst).

Nicola, I volunteer to send patches to make any necessary updates to
docs/misra/rules.rst and other docs. Please send out to me a list of
deviations/configurations and I'll make sure to reflect them under
docs/misra so that they are in sync.

Cheers,

Stefano

Re: [XEN PATCH 07/11] xen: address MISRA C:2012 Rule 2.1

[Nicola Vetrini]

> Jan has a point: I think we should record all our deviations and unique
> ways to interpret the rules under docs/misra. And the Eclair
> configuration should reflect that. It is not a good idea to only keep
> the information in the Eclair config because, even if it is now 
> upstream
> in xen.git, it is still a tool-specific configuration file -- it is not
> a proper document. MISRA C and its interpretation is important enough
> that we should have a plain English document to cover it (which now is
> docs/misra/rules.rst).
> 
> Nicola, I volunteer to send patches to make any necessary updates to
> docs/misra/rules.rst and other docs. Please send out to me a list of
> deviations/configurations and I'll make sure to reflect them under
> docs/misra so that they are in sync.
> 
> Cheers,
> 
> Stefano

Sure, I'll let you know when I have it.
Thanks,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)