[RFC PATCH 0/4] Add missing default labels to switch statements

[RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Hello, everybody!

We at EPAM Systems would like to present first series of patches targeting Xen
on ARM Functional Safety certification (ISO61508 based): implementation of
MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
default label as a measure of defensive programming technique.

Current patch series is against the source code used by Xen hypervisor on ARM
only and does not include Xen tools and x86 implementation. Although the target
is limited to ARM it does touch the code common to other architectures as well.

[1] https://www.misra.org.uk/

Oleksandr Andrushchenko (4):
  cert:arch/arm: Add missing default labels to switch statements
  cert:xen/common: Add missing default labels to switch statements
  cert:xen/drivers: Add missing default labels to switch statements
  cert:xen/xsm/flask: Add missing default labels to switch statements

 xen/arch/arm/decode.c              |  3 +++
 xen/arch/arm/domain.c              | 10 ++++++++++
 xen/arch/arm/guest_walk.c          |  2 ++
 xen/arch/arm/mm.c                  |  3 +++
 xen/arch/arm/p2m.c                 |  7 +++++++
 xen/arch/arm/traps.c               |  6 ++++++
 xen/arch/arm/vsmc.c                |  9 +++++++++
 xen/common/device_tree.c           |  2 ++
 xen/common/domain.c                |  4 ++++
 xen/common/efi/boot.c              |  2 ++
 xen/common/event_channel.c         |  5 +++++
 xen/common/grant_table.c           | 18 +++++++++++++++++-
 xen/common/kernel.c                |  6 ++++++
 xen/common/libelf/libelf-dominfo.c |  3 +++
 xen/common/libfdt/fdt.c            |  2 ++
 xen/common/libfdt/fdt_ro.c         |  3 +++
 xen/common/sched_arinc653.c        |  3 +++
 xen/common/sched_credit.c          |  3 +++
 xen/common/sched_credit2.c         |  3 +++
 xen/common/sched_rt.c              |  3 +++
 xen/common/trace.c                 |  2 ++
 xen/common/vsprintf.c              |  8 ++++++++
 xen/drivers/char/console.c         |  5 +++++
 xen/drivers/char/ns16550.c         |  2 ++
 xen/drivers/passthrough/arm/smmu.c |  9 +++++++++
 xen/xsm/flask/avc.c                |  2 ++
 xen/xsm/flask/hooks.c              |  4 ++++
 xen/xsm/flask/ss/services.c        |  5 +++++
 28 files changed, 133 insertions(+), 1 deletion(-)

-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 1/4] cert:arch/arm: Add missing default labels to switch statements

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

It is required by MISRA [1] that every switch statement has a default
label as a measure of defensive programming technique.

The changes in this patch are to match MISRA C:2012: Rule 16.4
requirements.

[1] https://www.misra.org.uk/

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/arch/arm/decode.c     |  3 +++
 xen/arch/arm/domain.c     | 10 ++++++++++
 xen/arch/arm/guest_walk.c |  2 ++
 xen/arch/arm/mm.c         |  3 +++
 xen/arch/arm/p2m.c        |  7 +++++++
 xen/arch/arm/traps.c      |  6 ++++++
 xen/arch/arm/vsmc.c       |  9 +++++++++
 7 files changed, 40 insertions(+)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 8b1e15d11892..1ed37696d678 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -112,6 +112,9 @@ static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
         case 3: /* Signed byte */
             update_dabt(dabt, reg, 0, true);
             break;
+        default:
+            ASSERT_UNREACHABLE();
+            goto bad_thumb;
         }
 
         break;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6dc633ed5048..ecb43736a7c3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -439,6 +439,11 @@ unsigned long hypercall_create_continuation(
                 case 3: regs->x3 = arg; break;
                 case 4: regs->x4 = arg; break;
                 case 5: regs->x5 = arg; break;
+                /*
+                 * arm_abi Hypercall Calling Convention:
+                 * A hypercall can take up to 5 arguments.
+                 */
+                default: BUG(); break;
                 }
             }
 
@@ -462,6 +467,11 @@ unsigned long hypercall_create_continuation(
                 case 3: regs->r3 = arg; break;
                 case 4: regs->r4 = arg; break;
                 case 5: regs->r5 = arg; break;
+                /*
+                 * arm_abi Hypercall Calling Convention:
+                 * A hypercall can take up to 5 arguments.
+                 */
+                default: BUG(); break;
                 }
             }
 
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 7db7a7321b20..8c4be32b7ef8 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -101,6 +101,8 @@ static bool guest_walk_sd(const struct vcpu *v,
 
     switch ( pte.walk.dt )
     {
+    default:
+        /* Fall through. */
     case L1DESC_INVALID:
         return false;
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 01ae2cccc068..ba5bf5b2b3ba 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1112,6 +1112,9 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
             pte.pt.xn = 0;
             pte.pt.ro = 1;
             break;
+        default:
+            pte.pt.valid = 0;
+            break;
         }
         write_pte(xen_xenmap + i, pte);
     }
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c38bd7e16e26..1e12dc0fd482 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -540,6 +540,10 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
     case p2m_max_real_type:
         BUG();
         break;
+
+    default:
+        BUG();
+        break;
     }
 
     /* Then restrict with access permissions */
@@ -574,6 +578,9 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
         e->p2m.read = e->p2m.write = 0;
         e->p2m.xn = 1;
         break;
+    default:
+        BUG();
+        break;
     }
 }
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8741aa1d59ce..42e1bd54d31f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1306,6 +1306,10 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
         show_execution_state(regs);
         panic("Assertion '%s' failed at %s%s:%d\n",
               predicate, prefix, filename, lineno);
+        break;
+
+    default:
+        break;
     }
 
     return -EINVAL;
@@ -1972,6 +1976,8 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
                 advance_pc(regs, hsr);
                 return;
             case IO_UNHANDLED:
+                /* Fall through. */
+            default:
                 /* IO unhandled, try another way to handle it. */
                 break;
             }
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index c72b9a04ff76..9eabed89f9c5 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -109,6 +109,8 @@ static bool handle_arch(struct cpu_user_regs *regs)
         case ARM_SMCCC_ARCH_WORKAROUND_2_FID:
             switch ( get_ssbd_state() )
             {
+            default:
+                /* Fall through. */
             case ARM_SSBD_UNKNOWN:
             case ARM_SSBD_FORCE_DISABLE:
                 break;
@@ -123,6 +125,8 @@ static bool handle_arch(struct cpu_user_regs *regs)
                 break;
             }
             break;
+        default:
+            break;
         }
 
         set_user_reg(regs, 0, ret);
@@ -152,6 +156,9 @@ static bool handle_arch(struct cpu_user_regs *regs)
 
         return true;
     }
+
+    default:
+        break;
     }
 
     return false;
@@ -276,6 +283,8 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
         case ARM_SMCCC_OWNER_SIP:
             handled = platform_smc(regs);
             break;
+        default:
+            break;
         }
     }
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 2/4] cert:xen/common: Add missing default labels to switch statements

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

It is required by MISRA [1] that every switch statement has a default
label as a measure of defensive programming technique.

The changes in this patch are to match MISRA C:2012: Rule 16.4
requirements.

[1] https://www.misra.org.uk/

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/common/device_tree.c           |  2 ++
 xen/common/domain.c                |  4 ++++
 xen/common/efi/boot.c              |  2 ++
 xen/common/event_channel.c         |  5 +++++
 xen/common/grant_table.c           | 18 +++++++++++++++++-
 xen/common/kernel.c                |  6 ++++++
 xen/common/libelf/libelf-dominfo.c |  3 +++
 xen/common/libfdt/fdt.c            |  2 ++
 xen/common/libfdt/fdt_ro.c         |  3 +++
 xen/common/sched_arinc653.c        |  3 +++
 xen/common/sched_credit.c          |  3 +++
 xen/common/sched_credit2.c         |  3 +++
 xen/common/sched_rt.c              |  3 +++
 xen/common/trace.c                 |  2 ++
 xen/common/vsprintf.c              |  8 ++++++++
 15 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8fc401d91cf0..342a62933ed6 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -568,6 +568,8 @@ static unsigned int dt_bus_pci_get_flags(const __be32 *addr)
     case 0x03: /* 64 bits */
         flags |= IORESOURCE_MEM;
         break;
+    default:
+        break;
     }
     if (w & 0x40000000)
         flags |= IORESOURCE_PREFETCH;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 32bca8dbf2df..9b253b167643 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -747,6 +747,8 @@ int domain_kill(struct domain *d)
         send_global_virq(VIRQ_DOM_EXC);
         /* fallthrough */
     case DOMDYING_dead:
+        /* fallthrough */
+    default:
         break;
     }
 
@@ -1569,6 +1571,8 @@ long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
     case VMASST_CMD_disable:
         clear_bit(type, &p->vm_assist);
         return 0;
+    default:
+        break;
     }
 
     return -ENOSYS;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 79193784ff14..673d4a625178 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -582,6 +582,8 @@ static CHAR16 *__init point_tail(CHAR16 *fn)
         case L'_':
             tail = fn;
             break;
+        default:
+            break;
         }
 }
 /*
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 6fbe346490fb..191a967c4b1d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -105,6 +105,8 @@ static bool virq_is_global(unsigned int virq)
 
     case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
         return arch_virq_is_global(virq);
+    default:
+        break;
     }
 
     ASSERT(virq < NR_VIRQS);
@@ -1419,6 +1421,9 @@ static void domain_dump_evtchn_info(struct domain *d)
         case ECS_VIRQ:
             printk(" v=%d", chn->u.virq);
             break;
+        default:
+            printk(" unknown state: %u", chn->state);
+            break;
         }
 
         ssid = xsm_show_security_evtchn(d, chn);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fd099a8f2593..0ed1c46c1790 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -640,6 +640,8 @@ static unsigned int nr_grant_entries(struct grant_table *gt)
                      GNTTAB_NR_RESERVED_ENTRIES);
         return f2e(nr_grant_frames(gt), 2);
 #undef f2e
+    default:
+        break;
     }
 
     return 0;
@@ -2992,7 +2994,9 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     case 2:
         for ( i = 0; i < GNTTAB_NR_RESERVED_ENTRIES; i++ )
         {
-            switch ( shared_entry_v2(gt, i).hdr.flags & GTF_type_mask )
+            unsigned int flags = shared_entry_v2(gt, i).hdr.flags;
+
+            switch ( flags & GTF_type_mask )
             {
             case GTF_permit_access:
                  if ( !(shared_entry_v2(gt, i).full_page.frame >> 32) )
@@ -3003,9 +3007,17 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
                          "tried to change grant table version to 1 with non-representable entries\n");
                 res = -ERANGE;
                 goto out_unlock;
+            default:
+                gdprintk(XENLOG_ERR,
+                         "bad flags %#x in grant %#x when switching version\n",
+                         flags, i);
+                res = -EINVAL;
+                goto out_unlock;
             }
         }
         break;
+    default:
+        break;
     }
 
     /* Preserve the first 8 entries (toolstack reserved grants). */
@@ -3038,6 +3050,8 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
             }
         }
         break;
+    default:
+        break;
     }
 
     if ( op.version < 2 && gt->gt_version == 2 &&
@@ -3069,6 +3083,8 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
                     reserved_entries[i].frame;
             }
             break;
+        default:
+            break;
         }
     }
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 612575430f1c..3983977b1643 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -267,6 +267,9 @@ int parse_bool(const char *s, const char *e)
         if ( !strncmp("disable", s, 7) )
             return 0;
         break;
+
+    default:
+        break;
     }
 
     return -1;
@@ -565,6 +568,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         return sz;
     }
+
+    default:
+        break;
     }
 
     return -ENOSYS;
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 508f08db428d..1f5acc2f71f8 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -216,6 +216,9 @@ elf_errorstatus elf_xen_parse_note(struct elf_binary *elf,
     case XEN_ELFNOTE_PHYS32_ENTRY:
         parms->phys_entry = val;
         break;
+
+    default:
+        break;
     }
     return 0;
 }
diff --git a/xen/common/libfdt/fdt.c b/xen/common/libfdt/fdt.c
index d02f4bf5f312..c30b45f8b49e 100644
--- a/xen/common/libfdt/fdt.c
+++ b/xen/common/libfdt/fdt.c
@@ -190,6 +190,8 @@ int fdt_next_node(const void *fdt, int offset, int *depth)
 				return -FDT_ERR_NOTFOUND;
 			else
 				return nextoffset;
+		default:
+			break;
 		}
 	} while (tag != FDT_BEGIN_NODE);
 
diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c
index 36f9b480d19e..bed77e195496 100644
--- a/xen/common/libfdt/fdt_ro.c
+++ b/xen/common/libfdt/fdt_ro.c
@@ -120,6 +120,9 @@ static int _nextprop(const void *fdt, int offset)
 
 		case FDT_PROP:
 			return offset;
+
+		default:
+			break;
 		}
 		offset = nextoffset;
 	} while (tag == FDT_NOP);
diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
index a4c6d00b8185..ba16b9e17f8c 100644
--- a/xen/common/sched_arinc653.c
+++ b/xen/common/sched_arinc653.c
@@ -689,6 +689,9 @@ a653sched_adjust_global(const struct scheduler *ops,
         if ( copy_to_guest(sc->u.sched_arinc653.schedule, &local_sched, 1) )
             rc = -EFAULT;
         break;
+
+    default:
+        break;
     }
 
     return rc;
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3abe20def842..b485d4c4fdf0 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1291,6 +1291,9 @@ csched_sys_cntl(const struct scheduler *ops,
         params->vcpu_migr_delay_us = prv->vcpu_migr_delay / MICROSECS(1);
         rc = 0;
         break;
+
+    default:
+        break;
     }
     out:
     return rc;
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 543dc3664dd3..927a70eab944 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -3037,6 +3037,9 @@ static int csched2_sys_cntl(const struct scheduler *ops,
     case XEN_SYSCTL_SCHEDOP_getinfo:
         params->ratelimit_us = prv->ratelimit_us;
         break;
+
+    default:
+        return -EINVAL;
     }
 
     return 0;
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index f1b81f037329..87db8bb68c9c 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1455,6 +1455,9 @@ rt_dom_cntl(
             /* notify upper caller how many vcpus have been processed. */
             op->u.v.nr_vcpus = index;
         break;
+    default:
+        rc = -EINVAL;
+        break;
     }
 
     return rc;
diff --git a/xen/common/trace.c b/xen/common/trace.c
index cc294fc38473..e345767552f8 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -863,6 +863,8 @@ void __trace_hypercall(uint32_t event, unsigned long op,
     case __HYPERVISOR_sched_op:
         APPEND_ARG32(0); /* cmd */
         break;
+    default:
+        break;
     }
 
     __trace_var(event, 1, sizeof(uint32_t) * (1 + (a - d.args)), &d);
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 352d43b42519..c2ee5ec06220 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -361,11 +361,13 @@ static char *print_domain(char *str, char *end, const struct domain *d)
     case DOMID_XEN:  name = "[XEN]";  break;
     case DOMID_COW:  name = "[COW]";  break;
     case DOMID_IDLE: name = "[IDLE]"; break;
+    default:
         /*
          * In principle, we could ASSERT_UNREACHABLE() in the default case.
          * However, this path is used to print out crash information, which
          * risks recursing infinitely and not printing any useful information.
          */
+        break;
     }
 
     if ( str < end )
@@ -455,6 +457,8 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
             ++*fmt_ptr;
             sep = 0;
             break;
+        default:
+            break;
         }
 
         for ( i = 0; ; )
@@ -519,6 +523,9 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
     case 'v': /* d<domain-id>v<vcpu-id> from a struct vcpu */
         ++*fmt_ptr;
         return print_vcpu(str, end, arg);
+
+    default:
+        break;
     }
 
     if ( field_width == -1 )
@@ -594,6 +601,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
         case ' ': flags |= SPACE; goto repeat;
         case '#': flags |= SPECIAL; goto repeat;
         case '0': flags |= ZEROPAD; goto repeat;
+        default : break;
         }
 
         /* get field width */
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 3/4] cert:xen/drivers: Add missing default labels to switch statements

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

It is required by MISRA [1] that every switch statement has a default
label as a measure of defensive programming technique.

The changes in this patch are to match MISRA C:2012: Rule 16.4
requirements.

[1] https://www.misra.org.uk/

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/drivers/char/console.c         | 5 +++++
 xen/drivers/char/ns16550.c         | 2 ++
 xen/drivers/passthrough/arm/smmu.c | 9 +++++++++
 3 files changed, 16 insertions(+)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 4315588f0597..e499ead90ca9 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -202,6 +202,7 @@ static char *loglvl_str(int lvl)
     case 2: return "Errors and warnings";
     case 3: return "Errors, warnings and info";
     case 4: return "All";
+    default: break;
     }
     return "???";
 }
@@ -708,6 +709,8 @@ static int printk_prefix_check(char *p, char **pp)
         case '0' ... '3':
             loglvl = p[1] - '0';
             break;
+        default:
+            break;
         }
         p += 3;
     }
@@ -732,6 +735,8 @@ static int parse_console_timestamps(const char *s)
     case 1:
         opt_con_timestamp_mode = TSM_DATE;
         return 0;
+    default:
+        break;
     }
     if ( *s == '\0' || /* Compat for old booleanparam() */
          !strcmp(s, "date") )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 189e121b7e4d..d0903a9d47db 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -995,6 +995,8 @@ static int __init parse_parity_char(int c)
         return UART_PARITY_MARK;
     case 's': 
         return UART_PARITY_SPACE;
+    default:
+        break;
     }
     return 0;
 }
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f5b5ee..cb35a7ce55ca 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1056,6 +1056,9 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 		case 48:
 			reg = (TTBCR2_ADDR_48 << TTBCR2_SEP_SHIFT);
 			break;
+		default:
+			BUG();
+			break;
 		}
 
 		switch (smmu->s1_output_size) {
@@ -1078,6 +1081,9 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 		case 48:
 			reg |= (TTBCR2_ADDR_48 << TTBCR2_PASIZE_SHIFT);
 			break;
+		default:
+			BUG();
+			break;
 		}
 
 		if (stage1)
@@ -1131,6 +1137,9 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 			case 48:
 				reg |= (TTBCR2_ADDR_48 << TTBCR_PASIZE_SHIFT);
 				break;
+			default:
+				BUG();
+				break;
 			}
 		} else {
 			reg |= (64 - smmu->s1_input_size) << TTBCR_T0SZ_SHIFT;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 4/4] cert:xen/xsm/flask: Add missing default labels to switch statements

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

It is required by MISRA [1] that every switch statement has a default
label as a measure of defensive programming technique.

The changes in this patch are to match MISRA C:2012: Rule 16.4
requirements.

[1] https://www.misra.org.uk/

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/xsm/flask/avc.c         | 2 ++
 xen/xsm/flask/hooks.c       | 4 ++++
 xen/xsm/flask/ss/services.c | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
index 640c708659b0..06ac404b0ad6 100644
--- a/xen/xsm/flask/avc.c
+++ b/xen/xsm/flask/avc.c
@@ -590,6 +590,8 @@ void avc_audit(u32 ssid, u32 tsid, u16 tclass, u32 requested,
     case AVC_AUDIT_DATA_DTDEV:
         avc_printk(&buf, "dtdevice=%s ", a->dtdev);
         break;
+    default:
+        break;
     }
 
     avc_dump_query(&buf, ssid, tsid, tclass);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6b7..f29c4a4ffd05 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -339,6 +339,8 @@ static char *flask_show_security_evtchn(struct domain *d, const struct evtchn *c
         if (irq && get_irq_sid(irq, &sid, NULL))
             return NULL;
         break;
+    default:
+        break;
     }
     if ( !sid )
         return NULL;
@@ -1246,6 +1248,8 @@ static int flask_hvm_altp2mhvm_op(struct domain *d, uint64_t mode, uint32_t op)
         if ( d == current->domain )
             return -EPERM;
         break;
+    default:
+        return -EPERM;
     };
 
     return current_has_perm(d, SECCLASS_HVM, HVM__ALTP2MHVM_OP);
diff --git a/xen/xsm/flask/ss/services.c b/xen/xsm/flask/ss/services.c
index b59928ea8a98..30dab4c35308 100644
--- a/xen/xsm/flask/ss/services.c
+++ b/xen/xsm/flask/ss/services.c
@@ -1018,6 +1018,11 @@ static int security_compute_sid(u32 ssid,
             /* Use the related object owner. */
             newcontext.user = tcontext->user;
         break;
+        default:
+            printk(KERN_ERR "security_compute_sid:  unrecognized AVTAB %d\n",
+                   specified);
+            rc = -EINVAL;
+            goto out_unlock;
     }
 
     /* Set the role and type to default values. */
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> Hello, everybody!
>
> We at EPAM Systems would like to present first series of patches targeting Xen
> on ARM Functional Safety certification (ISO61508 based): implementation of
> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
> default label as a measure of defensive programming technique.

Hang on - what?

Can someone attempt to justify why actively breaking -Wswitch is going
to result in safer/better code?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:
> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Hello, everybody!
>>
>> We at EPAM Systems would like to present first series of patches targeting Xen
>> on ARM Functional Safety certification (ISO61508 based): implementation of
>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>> default label as a measure of defensive programming technique.
> 
> Hang on - what?
> 
> Can someone attempt to justify why actively breaking -Wswitch is going
> to result in safer/better code?

I was about to ask the same. There are quite a few cases where this series is 
going to make more difficult extending enum.

Furthermore, using BUG() is a pretty bad idea in switch. A guest would be able 
to crash the whole platform if there was a coding mistake. Instead we should use 
ASSERT_UNREACHABLE() and provide proper fallback whenever it is possible.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/22/19 1:05 PM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 10:27, Andrew Cooper wrote:
>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Hello, everybody!
>>>
>>> We at EPAM Systems would like to present first series of patches 
>>> targeting Xen
>>> on ARM Functional Safety certification (ISO61508 based): 
>>> implementation of
>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch 
>>> statement has a
>>> default label as a measure of defensive programming technique.
>>
>> Hang on - what?
>>
>> Can someone attempt to justify why actively breaking -Wswitch is going
>> to result in safer/better code?
>
> I was about to ask the same. There are quite a few cases where this 
> series is going to make more difficult extending enum.
>
Well, I am not sure I can truly defend MISRA requirements here, but I'll try
to express my view on that.

Let us have a look at gcc's options [1]: so, for what you are saying:
"-Wswitch
Warn whenever a switch statement has an index of enumerated type and 
lacks a case for one or more of the named codes of that enumeration. 
(The presence of a default label prevents this warning.) case labels 
outside the enumeration range also provoke warnings when this option is 
used (even if there is a default label). This warning is enabled by -Wall."

So, if we do not have all the cases covered then this compiler's switch 
will fire
a warning (error). What if we have an integer as a switch's index, not 
an enumeration,
so then there is no easy way to handle all the cases and we have to 
provide *default* statement.

What if with time enumeration changes, what if some of the case 
statements get removed and so on.

All these, as per my understanding, lead to a defensive programming 
approach, e.g. do not rely on what
is here at the moment, but think that everything can change any time soon.

BTW, I checked the series with -Wswitch-default:
-Wswitch-default
Warn whenever a switch statement does not have a default case.
> Furthermore, using BUG() is a pretty bad idea in switch. 
It is and not only in the switch. The reason I put BUG is that I tried 
to follow
the existing "error handling" at those places.
Use of BUG() itself is another topic which will also need to be addressed
> A guest would be able to crash the whole platform if there was a 
> coding mistake. Instead we should use ASSERT_UNREACHABLE() and provide 
> proper fallback whenever it is possible.
Exactly, as I said we have to get rid of BUG() and friends and handle 
errors instead
>
> Cheers,
>
Thank you,
Oleksandr

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrii Anisov]

Hello Andrew,

On 22.02.19 12:27, Andrew Cooper wrote:
> Can someone attempt to justify why actively breaking -Wswitch is going
> to result in safer/better code?
I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under default label. Though at runtime, not compilation time.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrii Anisov]

Hello Andrew,

On 22.02.19 12:27, Andrew Cooper wrote:
> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Hello, everybody!
>>
>> We at EPAM Systems would like to present first series of patches targeting Xen
>> on ARM Functional Safety certification (ISO61508 based): implementation of
>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>> default label as a measure of defensive programming technique.
> 
> Hang on - what?
> 
> Can someone attempt to justify why actively breaking -Wswitch is going
> to result in safer/better code?

I would express my vision of that MISRA rule requirement:
It requires handling (in any meaning) all possible incoming values explicitly.
Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under default label. Though at runtime, not compilation time.

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Oleksandr,

On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:
> On 2/22/19 1:05 PM, Julien Grall wrote:
>> Hi,
>>
>> On 22/02/2019 10:27, Andrew Cooper wrote:
>>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> Hello, everybody!
>>>>
>>>> We at EPAM Systems would like to present first series of patches targeting Xen
>>>> on ARM Functional Safety certification (ISO61508 based): implementation of
>>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>>>> default label as a measure of defensive programming technique.
>>>
>>> Hang on - what?
>>>
>>> Can someone attempt to justify why actively breaking -Wswitch is going
>>> to result in safer/better code?
>>
>> I was about to ask the same. There are quite a few cases where this series is 
>> going to make more difficult extending enum.
>>
> Well, I am not sure I can truly defend MISRA requirements here, but I'll try
> to express my view on that.
> 
> Let us have a look at gcc's options [1]: so, for what you are saying:
> "-Wswitch
> Warn whenever a switch statement has an index of enumerated type and lacks a 
> case for one or more of the named codes of that enumeration. (The presence of a 
> default label prevents this warning.) case labels outside the enumeration range 
> also provoke warnings when this option is used (even if there is a default 
> label). This warning is enabled by -Wall."
> 
> So, if we do not have all the cases covered then this compiler's switch will fire
> a warning (error). What if we have an integer as a switch's index, not an 
> enumeration,
> so then there is no easy way to handle all the cases and we have to provide 
> *default* statement.

You have a point for the integer switch.

> 
> What if with time enumeration changes, what if some of the case statements get 
> removed and so on.

In that case, the compiler will throw an error (Xen is built with -Werror). So 
for enumeration the compiler will help us to spot the missing places.

If you add a default case, you remove us a good way to check we actually add the 
new element everywhere.

> 
> All these, as per my understanding, lead to a defensive programming approach, 
> e.g. do not rely on what
> is here at the moment, but think that everything can change any time soon.
It does not mean this is always the best solution. If we take the example of the 
enumeration, now you can't easily detect at compilation time you missed a case. 
This could potentially lead to painful debugging session to understand what is 
missing.

> 
> BTW, I checked the series with -Wswitch-default:
> -Wswitch-default
> Warn whenever a switch statement does not have a default case.
>> Furthermore, using BUG() is a pretty bad idea in switch. 
> It is and not only in the switch. The reason I put BUG is that I tried to follow
> the existing "error handling" at those places.

It is not because BUG() is been used today in some places that we need to 
continue to spread it.

> Use of BUG() itself is another topic which will also need to be addressed

So we should not add more of them...

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:21, Andrii Anisov wrote:
> On 22.02.19 12:27, Andrew Cooper wrote:
>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Hello, everybody!
>>>
>>> We at EPAM Systems would like to present first series of patches targeting Xen
>>> on ARM Functional Safety certification (ISO61508 based): implementation of
>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>>> default label as a measure of defensive programming technique.
>>
>> Hang on - what?
>>
>> Can someone attempt to justify why actively breaking -Wswitch is going
>> to result in safer/better code?
> 
> I would express my vision of that MISRA rule requirement:
> It requires handling (in any meaning) all possible incoming values explicitly.
> Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
> default label. Though at runtime, not compilation time.

While review tend to be very thorough, it is sometimes hard to spot when we miss 
a case. This is where -Wswitch comes into place to spot missing how.

How the BUG/ASSERT_UNREACHABLE solution is going to help us here?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:30, Julien Grall wrote:
> 
> 
> On 22/02/2019 11:21, Andrii Anisov wrote:
>> On 22.02.19 12:27, Andrew Cooper wrote:
>>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>
>>>> Hello, everybody!
>>>>
>>>> We at EPAM Systems would like to present first series of patches targeting Xen
>>>> on ARM Functional Safety certification (ISO61508 based): implementation of
>>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>>>> default label as a measure of defensive programming technique.
>>>
>>> Hang on - what?
>>>
>>> Can someone attempt to justify why actively breaking -Wswitch is going
>>> to result in safer/better code?
>>
>> I would express my vision of that MISRA rule requirement:
>> It requires handling (in any meaning) all possible incoming values explicitly.
>> Switching through enum still could be guarded by BUG/ASSERT_UNREACHABLE under 
>> default label. Though at runtime, not compilation time.
> 
> While review tend to be very thorough, it is sometimes hard to spot when we miss 
> a case. This is where -Wswitch comes into place to spot missing how.

s/how/case/

> 
> How the BUG/ASSERT_UNREACHABLE solution is going to help us here?
> 
> Cheers,
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrii Anisov]

Hello Julien,

On 22.02.19 13:30, Julien Grall wrote:
> While review tend to be very thorough, it is sometimes hard to spot when we miss a case. This is where -Wswitch comes into place to spot missing how.
>
> How the BUG/ASSERT_UNREACHABLE solution is going to help us here?
I understand that hitting -Wswitch requires less efforts, the compilation only. Otherwise build run, even fulfilling some conditions are needed to hit missed enum index.

But how those `switch()` without `default:` label are protected against the value outside the enum?

-- 
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/22/19 1:27 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:
>> On 2/22/19 1:05 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/02/2019 10:27, Andrew Cooper wrote:
>>>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>>
>>>>> Hello, everybody!
>>>>>
>>>>> We at EPAM Systems would like to present first series of patches 
>>>>> targeting Xen
>>>>> on ARM Functional Safety certification (ISO61508 based): 
>>>>> implementation of
>>>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch 
>>>>> statement has a
>>>>> default label as a measure of defensive programming technique.
>>>>
>>>> Hang on - what?
>>>>
>>>> Can someone attempt to justify why actively breaking -Wswitch is going
>>>> to result in safer/better code?
>>>
>>> I was about to ask the same. There are quite a few cases where this 
>>> series is going to make more difficult extending enum.
>>>
>> Well, I am not sure I can truly defend MISRA requirements here, but 
>> I'll try
>> to express my view on that.
>>
>> Let us have a look at gcc's options [1]: so, for what you are saying:
>> "-Wswitch
>> Warn whenever a switch statement has an index of enumerated type and 
>> lacks a case for one or more of the named codes of that enumeration. 
>> (The presence of a default label prevents this warning.) case labels 
>> outside the enumeration range also provoke warnings when this option 
>> is used (even if there is a default label). This warning is enabled 
>> by -Wall."
>>
>> So, if we do not have all the cases covered then this compiler's 
>> switch will fire
>> a warning (error). What if we have an integer as a switch's index, 
>> not an enumeration,
>> so then there is no easy way to handle all the cases and we have to 
>> provide *default* statement.
>
> You have a point for the integer switch.
>
>>
>> What if with time enumeration changes, what if some of the case 
>> statements get removed and so on.
>
> In that case, the compiler will throw an error (Xen is built with 
> -Werror). So for enumeration the compiler will help us to spot the 
> missing places.
>
> If you add a default case, you remove us a good way to check we 
> actually add the new element everywhere.
The answer is that what happens if we by any reason either by a mistake 
or any other mean
have a build which doesn't have -Wswitch or -Wall. I mean that 
everything changes and having
"default" in the source does guarantee the handling as it was intended 
to be.
>
>>
>> All these, as per my understanding, lead to a defensive programming 
>> approach, e.g. do not rely on what
>> is here at the moment, but think that everything can change any time 
>> soon.
> It does not mean this is always the best solution. If we take the 
> example of the enumeration, now you can't easily detect at compilation 
> time you missed a case. This could potentially lead to painful 
> debugging session to understand what is missing.
>
But what happens if you miss default handling and because of bugs in the 
code you do not
handle "compile time unexpected values"?
>>
>> BTW, I checked the series with -Wswitch-default:
>> -Wswitch-default
>> Warn whenever a switch statement does not have a default case.
>>> Furthermore, using BUG() is a pretty bad idea in switch. 
>> It is and not only in the switch. The reason I put BUG is that I 
>> tried to follow
>> the existing "error handling" at those places.
>
> It is not because BUG() is been used today in some places that we need 
> to continue to spread it.
>
>> Use of BUG() itself is another topic which will also need to be 
>> addressed
>
> So we should not add more of them...
Again, I see this as a dedicated change. So, in the current series I 
think it is
acceptable to use the existing way of error handling if any at all.
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 12:01, Oleksandr Andrushchenko wrote:
> On 2/22/19 1:27 PM, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 22/02/2019 11:13, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 1:05 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 10:27, Andrew Cooper wrote:
>>>>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>>>
>>>>>> Hello, everybody!
>>>>>>
>>>>>> We at EPAM Systems would like to present first series of patches targeting 
>>>>>> Xen
>>>>>> on ARM Functional Safety certification (ISO61508 based): implementation of
>>>>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>>>>>> default label as a measure of defensive programming technique.
>>>>>
>>>>> Hang on - what?
>>>>>
>>>>> Can someone attempt to justify why actively breaking -Wswitch is going
>>>>> to result in safer/better code?
>>>>
>>>> I was about to ask the same. There are quite a few cases where this series 
>>>> is going to make more difficult extending enum.
>>>>
>>> Well, I am not sure I can truly defend MISRA requirements here, but I'll try
>>> to express my view on that.
>>>
>>> Let us have a look at gcc's options [1]: so, for what you are saying:
>>> "-Wswitch
>>> Warn whenever a switch statement has an index of enumerated type and lacks a 
>>> case for one or more of the named codes of that enumeration. (The presence of 
>>> a default label prevents this warning.) case labels outside the enumeration 
>>> range also provoke warnings when this option is used (even if there is a 
>>> default label). This warning is enabled by -Wall."
>>>
>>> So, if we do not have all the cases covered then this compiler's switch will 
>>> fire
>>> a warning (error). What if we have an integer as a switch's index, not an 
>>> enumeration,
>>> so then there is no easy way to handle all the cases and we have to provide 
>>> *default* statement.
>>
>> You have a point for the integer switch.
>>
>>>
>>> What if with time enumeration changes, what if some of the case statements 
>>> get removed and so on.
>>
>> In that case, the compiler will throw an error (Xen is built with -Werror). So 
>> for enumeration the compiler will help us to spot the missing places.
>>
>> If you add a default case, you remove us a good way to check we actually add 
>> the new element everywhere.
> The answer is that what happens if we by any reason either by a mistake or any 
> other mean
> have a build which doesn't have -Wswitch or -Wall. I mean that everything 
> changes and having
> "default" in the source does guarantee the handling as it was intended to be.

If you consider that and ...
[...]

>>
> But what happens if you miss default handling and because of bugs in the code 
> you do not
> handle "compile time unexpected values"?
that. Then when do you put a limit between theoretical and real issue? For 
instance, what is the default case is introduced an unintended behavior?

>>>
>>> BTW, I checked the series with -Wswitch-default:
>>> -Wswitch-default
>>> Warn whenever a switch statement does not have a default case.
>>>> Furthermore, using BUG() is a pretty bad idea in switch. 
>>> It is and not only in the switch. The reason I put BUG is that I tried to follow
>>> the existing "error handling" at those places.
>>
>> It is not because BUG() is been used today in some places that we need to 
>> continue to spread it.
>>
>>> Use of BUG() itself is another topic which will also need to be addressed
>>
>> So we should not add more of them...
> Again, I see this as a dedicated change. So, in the current series I think it is
> acceptable to use the existing way of error handling if any at all.

That's not how it works in upstream. If you know some constructs are wrong, it 
is best to try to address partially the problem directly then having so you 
reduce the amounts of change afterwards.

So please try to not introduce more BUG() in the code base.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 11:53, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 22.02.19 13:30, Julien Grall wrote:
>> While review tend to be very thorough, it is sometimes hard to spot when we 
>> miss a case. This is where -Wswitch comes into place to spot missing how.
>>
>> How the BUG/ASSERT_UNREACHABLE solution is going to help us here?
> I understand that hitting -Wswitch requires less efforts, the compilation only. 
> Otherwise build run, even fulfilling some conditions are needed to hit missed 
> enum index.
> 
> But how those `switch()` without `default:` label are protected against the 
> value outside the enum?

You really don't need a default case for handling such issues. You can construct 
code in a way to initialize the variables with default values.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 1/4] cert:arch/arm: Add missing default labels to switch statements

[Julien Grall]

Hi Oleksandr,

On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> It is required by MISRA [1] that every switch statement has a default
> label as a measure of defensive programming technique.
> 
> The changes in this patch are to match MISRA C:2012: Rule 16.4
> requirements.
> 
> [1] https://www.misra.org.uk/
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   xen/arch/arm/decode.c     |  3 +++
>   xen/arch/arm/domain.c     | 10 ++++++++++
>   xen/arch/arm/guest_walk.c |  2 ++
>   xen/arch/arm/mm.c         |  3 +++
>   xen/arch/arm/p2m.c        |  7 +++++++
>   xen/arch/arm/traps.c      |  6 ++++++
>   xen/arch/arm/vsmc.c       |  9 +++++++++
>   7 files changed, 40 insertions(+)
> 
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> index 8b1e15d11892..1ed37696d678 100644
> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -112,6 +112,9 @@ static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>           case 3: /* Signed byte */
>               update_dabt(dabt, reg, 0, true);
>               break;
> +        default:
> +            ASSERT_UNREACHABLE();
> +            goto bad_thumb;

Here the switch can only have 4 value (see opB & 0x3). They are handled 
correctly, so not only it does not make much sense to me and it adds more 
confusion to it.

>           }
>   
>           break;
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6dc633ed5048..ecb43736a7c3 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -439,6 +439,11 @@ unsigned long hypercall_create_continuation(
>                   case 3: regs->x3 = arg; break;
>                   case 4: regs->x4 = arg; break;
>                   case 5: regs->x5 = arg; break;
> +                /*
> +                 * arm_abi Hypercall Calling Convention:

s/arm_abi/ARM ABI/

> +                 * A hypercall can take up to 5 arguments.
> +                 */
> +                default: BUG(); break;

This should be an ASSERT_UNREACHABLE here.

>                   }
>               }
>   
> @@ -462,6 +467,11 @@ unsigned long hypercall_create_continuation(
>                   case 3: regs->r3 = arg; break;
>                   case 4: regs->r4 = arg; break;
>                   case 5: regs->r5 = arg; break;
> +                /*
> +                 * arm_abi Hypercall Calling Convention:

Ditto.
> +                 * A hypercall can take up to 5 arguments.
> +                 */
> +                default: BUG(); break;

Ditto.

>                   }
>               }
>   
> diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
> index 7db7a7321b20..8c4be32b7ef8 100644
> --- a/xen/arch/arm/guest_walk.c
> +++ b/xen/arch/arm/guest_walk.c
> @@ -101,6 +101,8 @@ static bool guest_walk_sd(const struct vcpu *v,
>   
>       switch ( pte.walk.dt )
>       {
> +    default:
> +        /* Fall through. */

Similar to the first switch, we cover all the values here. So what does it 
really bring us?

>       case L1DESC_INVALID:
>           return false;
>   
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 01ae2cccc068..ba5bf5b2b3ba 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1112,6 +1112,9 @@ static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>               pte.pt.xn = 0;
>               pte.pt.ro = 1;
>               break;
> +        default:
> +            pte.pt.valid = 0;
> +            break;

This one, ...

>           }
>           write_pte(xen_xenmap + i, pte);
>       }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index c38bd7e16e26..1e12dc0fd482 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -540,6 +540,10 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
>       case p2m_max_real_type:
>           BUG();
>           break;
> +
> +    default:
> +        BUG();
> +        break;

... this one and...

>       }
>   
>       /* Then restrict with access permissions */
> @@ -574,6 +578,9 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
>           e->p2m.read = e->p2m.write = 0;
>           e->p2m.xn = 1;
>           break;
> +    default:
> +        BUG();
> +        break;

... this one is going to make much harder to extend the enum. TBH, I don't see 
the issue of initializing before the switch with default invalid value and don't 
add the default here.

>       }
>   }
>   
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 8741aa1d59ce..42e1bd54d31f 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1306,6 +1306,10 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>           show_execution_state(regs);
>           panic("Assertion '%s' failed at %s%s:%d\n",
>                 predicate, prefix, filename, lineno);
> +        break;
> +
> +    default:
> +        break;

As all the other case panic or return an error, you can move "return -EINVAL" here.

>       }
>   
>       return -EINVAL;
> @@ -1972,6 +1976,8 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
>                   advance_pc(regs, hsr);
>                   return;
>               case IO_UNHANDLED:
> +                /* Fall through. */
> +            default:
>                   /* IO unhandled, try another way to handle it. */
>                   break;
>               }
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index c72b9a04ff76..9eabed89f9c5 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -109,6 +109,8 @@ static bool handle_arch(struct cpu_user_regs *regs)
>           case ARM_SMCCC_ARCH_WORKAROUND_2_FID:
>               switch ( get_ssbd_state() )
>               {
> +            default:
> +                /* Fall through. */

Similar question to the p2m case here.

>               case ARM_SSBD_UNKNOWN:
>               case ARM_SSBD_FORCE_DISABLE:
>                   break;
> @@ -123,6 +125,8 @@ static bool handle_arch(struct cpu_user_regs *regs)
>                   break;
>               }
>               break;
> +        default:
> +            break;

This kind of construct is very questionable. It adds 2 lines for the only 
benefits to tell MISRA tools to shut up.

>           }
>   
>           set_user_reg(regs, 0, ret);
> @@ -152,6 +156,9 @@ static bool handle_arch(struct cpu_user_regs *regs)
>   
>           return true;
>       }
> +
> +    default:
> +        break;

Same here.

>       }
>   
>       return false;
> @@ -276,6 +283,8 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>           case ARM_SMCCC_OWNER_SIP:
>               handled = platform_smc(regs);
>               break;
> +        default:
> +            break;

Same here.

>           }
>       }
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Fri, 22 Feb 2019, Julien Grall wrote:
> > > > BTW, I checked the series with -Wswitch-default:
> > > > -Wswitch-default
> > > > Warn whenever a switch statement does not have a default case.
> > > > > Furthermore, using BUG() is a pretty bad idea in switch. 
> > > > It is and not only in the switch. The reason I put BUG is that I tried
> > > > to follow
> > > > the existing "error handling" at those places.
> > > 
> > > It is not because BUG() is been used today in some places that we need to
> > > continue to spread it.
> > > 
> > > > Use of BUG() itself is another topic which will also need to be
> > > > addressed
> > > 
> > > So we should not add more of them...
> > Again, I see this as a dedicated change. So, in the current series I think
> > it is
> > acceptable to use the existing way of error handling if any at all.
> 
> That's not how it works in upstream. If you know some constructs are wrong, it
> is best to try to address partially the problem directly then having so you
> reduce the amounts of change afterwards.
> 
> So please try to not introduce more BUG() in the code base.

Hi Oleksandr, Julien,

Julien's right that we should not introduce any more BUG()s. In fact,
each of them makes the code less safe, not more safe! The purpose of
MISRAC 16.4 is "defensive programming": write the code in a way that is
more (not less!) resilient to failure.

So, I think it is a good idea to introduce a default label because it
can help us spot unexpected issues. Instead of calling BUG() in the
default handler, which is detrimental, we should return an error when
possible, or just print a warning.

As 16.4 clearly state, even a simple comment would be enough to address
the rule. We just need to explain why a default label is not needed.
Such as:

  default:
  /* unreachable because blah and blah */


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>> BTW, I checked the series with -Wswitch-default:
>>>>> -Wswitch-default
>>>>> Warn whenever a switch statement does not have a default case.
>>>>>> Furthermore, using BUG() is a pretty bad idea in switch. 
>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>> to follow
>>>>> the existing "error handling" at those places.
>>>> It is not because BUG() is been used today in some places that we need to
>>>> continue to spread it.
>>>>
>>>>> Use of BUG() itself is another topic which will also need to be
>>>>> addressed
>>>> So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>> That's not how it works in upstream. If you know some constructs are wrong, it
>> is best to try to address partially the problem directly then having so you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> Hi Oleksandr, Julien,
>
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
>
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

domain_crash() is almost always better than BUG().  It is very obvious
if it gets hit, and wont crash Xen.

>
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
>
>   default:
>   /* unreachable because blah and blah */

What a simple comment doesn't do is avoid breaking -Wswitch.

This requirement is actively hostile towards compilers trying to help
you spot when you made a mistake and forgot to update one of the $N
places you needed to.

In this case, I don't think "Because MISRA demand it" is a good enough
justification to offset the increased error-prone-ness of the result.

~Andrew

P.S. There is a solution here which could work, but IMO a better use of
time and energy would be to get MISRA to update their rules to match
this century, and stop getting in the way of compiler features intended
to help the programmer avoid bugs.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3163 bytes --]

On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Julien Grall wrote:
> >>>>> BTW, I checked the series with -Wswitch-default:
> >>>>> -Wswitch-default
> >>>>> Warn whenever a switch statement does not have a default case.
> >>>>>> Furthermore, using BUG() is a pretty bad idea in switch. 
> >>>>> It is and not only in the switch. The reason I put BUG is that I tried
> >>>>> to follow
> >>>>> the existing "error handling" at those places.
> >>>> It is not because BUG() is been used today in some places that we need to
> >>>> continue to spread it.
> >>>>
> >>>>> Use of BUG() itself is another topic which will also need to be
> >>>>> addressed
> >>>> So we should not add more of them...
> >>> Again, I see this as a dedicated change. So, in the current series I think
> >>> it is
> >>> acceptable to use the existing way of error handling if any at all.
> >> That's not how it works in upstream. If you know some constructs are wrong, it
> >> is best to try to address partially the problem directly then having so you
> >> reduce the amounts of change afterwards.
> >>
> >> So please try to not introduce more BUG() in the code base.
> > Hi Oleksandr, Julien,
> >
> > Julien's right that we should not introduce any more BUG()s. In fact,
> > each of them makes the code less safe, not more safe! The purpose of
> > MISRAC 16.4 is "defensive programming": write the code in a way that is
> > more (not less!) resilient to failure.
> >
> > So, I think it is a good idea to introduce a default label because it
> > can help us spot unexpected issues. Instead of calling BUG() in the
> > default handler, which is detrimental, we should return an error when
> > possible, or just print a warning.
> 
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.

That's a good suggestion.


> > As 16.4 clearly state, even a simple comment would be enough to address
> > the rule. We just need to explain why a default label is not needed.
> > Such as:
> >
> >   default:
> >   /* unreachable because blah and blah */
> 
> What a simple comment doesn't do is avoid breaking -Wswitch.

I don't know how to reconcile 16.4 with -Wswitch. One could argue that
-Wswitch could be a good way to address 16.4, but then we introduce a
compiler specific requirement. Typically gcc is not the compiler of
choice for these environments, unfortunately forcing gcc is not an
option.

But if there was a non-gcc way to do -Wswitch, then yes, that would be
the way to go.


> This requirement is actively hostile towards compilers trying to help
> you spot when you made a mistake and forgot to update one of the $N
> places you needed to.
> 
> In this case, I don't think "Because MISRA demand it" is a good enough
> justification to offset the increased error-prone-ness of the result.
> 
> ~Andrew
> 
> P.S. There is a solution here which could work, but IMO a better use of
> time and energy would be to get MISRA to update their rules to match
> this century, and stop getting in the way of compiler features intended
> to help the programmer avoid bugs.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

On 22/02/2019 21:00, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>> BTW, I checked the series with -Wswitch-default:
>>>>> -Wswitch-default
>>>>> Warn whenever a switch statement does not have a default case.
>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>> to follow
>>>>> the existing "error handling" at those places.
>>>>
>>>> It is not because BUG() is been used today in some places that we need to
>>>> continue to spread it.
>>>>
>>>>> Use of BUG() itself is another topic which will also need to be
>>>>> addressed
>>>>
>>>> So we should not add more of them...
>>> Again, I see this as a dedicated change. So, in the current series I think
>>> it is
>>> acceptable to use the existing way of error handling if any at all.
>>
>> That's not how it works in upstream. If you know some constructs are wrong, it
>> is best to try to address partially the problem directly then having so you
>> reduce the amounts of change afterwards.
>>
>> So please try to not introduce more BUG() in the code base.
> 
> Hi Oleksandr, Julien,
> 
> Julien's right that we should not introduce any more BUG()s. In fact,
> each of them makes the code less safe, not more safe! The purpose of
> MISRAC 16.4 is "defensive programming": write the code in a way that is
> more (not less!) resilient to failure.
> 
> So, I think it is a good idea to introduce a default label because it
> can help us spot unexpected issues. Instead of calling BUG() in the
> default handler, which is detrimental, we should return an error when
> possible, or just print a warning.

I looked at the first patch and to be honest I can't see how this hence 
our code...

> 
> As 16.4 clearly state, even a simple comment would be enough to address
> the rule. We just need to explain why a default label is not needed.
> Such as:
> 
>    default:
>    /* unreachable because blah and blah */

... as we already have defensive code. Indeed, in most of the switch we 
deal with potential issue by initializing the variable before the 
switch. If you look at the first patch, a lot of "default: break;" is 
introduced. So what's our benefits? How this code is more defensive than 
what we currently have?

Furthermore, how this is going to help us (thanks to -Wswitch) if an 
enumerate is extended and we miss a case that the compiler don't notice 
anymore?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Stefano,

On 22/02/2019 21:58, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>> -Wswitch-default
>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>> to follow
>>>>>>> the existing "error handling" at those places.
>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>> continue to spread it.
>>>>>>
>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>> addressed
>>>>>> So we should not add more of them...
>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>> it is
>>>>> acceptable to use the existing way of error handling if any at all.
>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>> is best to try to address partially the problem directly then having so you
>>>> reduce the amounts of change afterwards.
>>>>
>>>> So please try to not introduce more BUG() in the code base.
>>> Hi Oleksandr, Julien,
>>>
>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>> each of them makes the code less safe, not more safe! The purpose of
>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>> more (not less!) resilient to failure.
>>>
>>> So, I think it is a good idea to introduce a default label because it
>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>> default handler, which is detrimental, we should return an error when
>>> possible, or just print a warning.
>>
>> domain_crash() is almost always better than BUG().  It is very obvious
>> if it gets hit, and wont crash Xen.
> 
> That's a good suggestion.
> 
> 
>>> As 16.4 clearly state, even a simple comment would be enough to address
>>> the rule. We just need to explain why a default label is not needed.
>>> Such as:
>>>
>>>    default:
>>>    /* unreachable because blah and blah */
>>
>> What a simple comment doesn't do is avoid breaking -Wswitch.
> 
> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> -Wswitch could be a good way to address 16.4, but then we introduce a
> compiler specific requirement. Typically gcc is not the compiler of
> choice for these environments, unfortunately forcing gcc is not an
> option.

Well, you could build with GCC and then build with your custom 
compiler... But, GCC is pretty much the only choice for Xen on Arm today 
as we don't build with clang and I pretty doubt we can build with compcert.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3835 bytes --]

On Fri, 22 Feb 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/02/2019 21:58, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Andrew Cooper wrote:
> >> On 22/02/2019 21:00, Stefano Stabellini wrote:
> >>> On Fri, 22 Feb 2019, Julien Grall wrote:
> >>>>>>> BTW, I checked the series with -Wswitch-default:
> >>>>>>> -Wswitch-default
> >>>>>>> Warn whenever a switch statement does not have a default case.
> >>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
> >>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
> >>>>>>> to follow
> >>>>>>> the existing "error handling" at those places.
> >>>>>> It is not because BUG() is been used today in some places that we need to
> >>>>>> continue to spread it.
> >>>>>>
> >>>>>>> Use of BUG() itself is another topic which will also need to be
> >>>>>>> addressed
> >>>>>> So we should not add more of them...
> >>>>> Again, I see this as a dedicated change. So, in the current series I think
> >>>>> it is
> >>>>> acceptable to use the existing way of error handling if any at all.
> >>>> That's not how it works in upstream. If you know some constructs are wrong, it
> >>>> is best to try to address partially the problem directly then having so you
> >>>> reduce the amounts of change afterwards.
> >>>>
> >>>> So please try to not introduce more BUG() in the code base.
> >>> Hi Oleksandr, Julien,
> >>>
> >>> Julien's right that we should not introduce any more BUG()s. In fact,
> >>> each of them makes the code less safe, not more safe! The purpose of
> >>> MISRAC 16.4 is "defensive programming": write the code in a way that is
> >>> more (not less!) resilient to failure.
> >>>
> >>> So, I think it is a good idea to introduce a default label because it
> >>> can help us spot unexpected issues. Instead of calling BUG() in the
> >>> default handler, which is detrimental, we should return an error when
> >>> possible, or just print a warning.
> >>
> >> domain_crash() is almost always better than BUG().  It is very obvious
> >> if it gets hit, and wont crash Xen.
> > 
> > That's a good suggestion.
> > 
> > 
> >>> As 16.4 clearly state, even a simple comment would be enough to address
> >>> the rule. We just need to explain why a default label is not needed.
> >>> Such as:
> >>>
> >>>    default:
> >>>    /* unreachable because blah and blah */
> >>
> >> What a simple comment doesn't do is avoid breaking -Wswitch.
> > 
> > I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> > -Wswitch could be a good way to address 16.4, but then we introduce a
> > compiler specific requirement. Typically gcc is not the compiler of
> > choice for these environments, unfortunately forcing gcc is not an
> > option.
> 
> Well, you could build with GCC and then build with your custom 
> compiler... 

This suggestion is problematic: as an individual interested in MISRA-C
compliance, I only have the MISRA-C rules in my hands. I don't know how
to deal with suggestions like this one, that don't comply to the Rules,
but it tries to address the same issue in a different manner.

I cannot rule out that it wouldn't work, but also I cannot be sure that
it would work. In short, I have no way to make progress or to find out
how to move forward. I guess as a contributor I would be forced to go
back to the MISRAC compliance experts and ask for their opinion. (One
non-technical issue is who is going to pay them for spending their time
on this.) But what if they say it is not acceptable for compliance?

This is a great topic to discuss in March and decide what to do in these
situations.


> But, GCC is pretty much the only choice for Xen on Arm today 
> as we don't build with clang and I pretty doubt we can build with compcert.

Obviously, this has to change if we want to make progress on safety
certifications.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 22:11, Julien Grall wrote:
> Hi Stefano,
>
> On 22/02/2019 21:58, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>> -Wswitch-default
>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>> to follow
>>>>>>>> the existing "error handling" at those places.
>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>> continue to spread it.
>>>>>>>
>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>> addressed
>>>>>>> So we should not add more of them...
>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>> it is
>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>> is best to try to address partially the problem directly then having so you
>>>>> reduce the amounts of change afterwards.
>>>>>
>>>>> So please try to not introduce more BUG() in the code base.
>>>> Hi Oleksandr, Julien,
>>>>
>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>> each of them makes the code less safe, not more safe! The purpose of
>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>> more (not less!) resilient to failure.
>>>>
>>>> So, I think it is a good idea to introduce a default label because it
>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>> default handler, which is detrimental, we should return an error when
>>>> possible, or just print a warning.
>>> domain_crash() is almost always better than BUG().  It is very obvious
>>> if it gets hit, and wont crash Xen.
>> That's a good suggestion.
>>
>>
>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>> the rule. We just need to explain why a default label is not needed.
>>>> Such as:
>>>>
>>>>    default:
>>>>    /* unreachable because blah and blah */
>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>> -Wswitch could be a good way to address 16.4, but then we introduce a
>> compiler specific requirement. Typically gcc is not the compiler of
>> choice for these environments, unfortunately forcing gcc is not an
>> option.
> Well, you could build with GCC and then build with your custom 
> compiler... But, GCC is pretty much the only choice for Xen on Arm today 
> as we don't build with clang and I pretty doubt we can build with compcert.

So the suggestion I had was to have an overall CONFIG_MISRA which we can
hide some of this nonsense behind, and then

#ifdef CONFIG_MISRA
#define MISRA_BLE_DEFAULT default:
#else
#define MISRA_BLE_DEFAULT
#endif

So when you disable CONFIG_MISRA, your compiler starts being able to
help you again.

TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
continue doing it the efficient way for ~100% of the time.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3676 bytes --]

On Fri, 22 Feb 2019, Andrew Cooper wrote:
> On 22/02/2019 22:11, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 22/02/2019 21:58, Stefano Stabellini wrote:
> >> On Fri, 22 Feb 2019, Andrew Cooper wrote:
> >>> On 22/02/2019 21:00, Stefano Stabellini wrote:
> >>>> On Fri, 22 Feb 2019, Julien Grall wrote:
> >>>>>>>> BTW, I checked the series with -Wswitch-default:
> >>>>>>>> -Wswitch-default
> >>>>>>>> Warn whenever a switch statement does not have a default case.
> >>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
> >>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
> >>>>>>>> to follow
> >>>>>>>> the existing "error handling" at those places.
> >>>>>>> It is not because BUG() is been used today in some places that we need to
> >>>>>>> continue to spread it.
> >>>>>>>
> >>>>>>>> Use of BUG() itself is another topic which will also need to be
> >>>>>>>> addressed
> >>>>>>> So we should not add more of them...
> >>>>>> Again, I see this as a dedicated change. So, in the current series I think
> >>>>>> it is
> >>>>>> acceptable to use the existing way of error handling if any at all.
> >>>>> That's not how it works in upstream. If you know some constructs are wrong, it
> >>>>> is best to try to address partially the problem directly then having so you
> >>>>> reduce the amounts of change afterwards.
> >>>>>
> >>>>> So please try to not introduce more BUG() in the code base.
> >>>> Hi Oleksandr, Julien,
> >>>>
> >>>> Julien's right that we should not introduce any more BUG()s. In fact,
> >>>> each of them makes the code less safe, not more safe! The purpose of
> >>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
> >>>> more (not less!) resilient to failure.
> >>>>
> >>>> So, I think it is a good idea to introduce a default label because it
> >>>> can help us spot unexpected issues. Instead of calling BUG() in the
> >>>> default handler, which is detrimental, we should return an error when
> >>>> possible, or just print a warning.
> >>> domain_crash() is almost always better than BUG().  It is very obvious
> >>> if it gets hit, and wont crash Xen.
> >> That's a good suggestion.
> >>
> >>
> >>>> As 16.4 clearly state, even a simple comment would be enough to address
> >>>> the rule. We just need to explain why a default label is not needed.
> >>>> Such as:
> >>>>
> >>>>    default:
> >>>>    /* unreachable because blah and blah */
> >>> What a simple comment doesn't do is avoid breaking -Wswitch.
> >> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> >> -Wswitch could be a good way to address 16.4, but then we introduce a
> >> compiler specific requirement. Typically gcc is not the compiler of
> >> choice for these environments, unfortunately forcing gcc is not an
> >> option.
> > Well, you could build with GCC and then build with your custom 
> > compiler... But, GCC is pretty much the only choice for Xen on Arm today 
> > as we don't build with clang and I pretty doubt we can build with compcert.
> 
> So the suggestion I had was to have an overall CONFIG_MISRA which we can
> hide some of this nonsense behind, and then
> 
> #ifdef CONFIG_MISRA
> #define MISRA_BLE_DEFAULT default:
> #else
> #define MISRA_BLE_DEFAULT
> #endif
> 
> So when you disable CONFIG_MISRA, your compiler starts being able to
> help you again.

This is actually a good way to make progress which could make everybody
happy. (And it wouldn't require a slow back-and-forth with third parties
to ask difficult questions.)


> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
> continue doing it the efficient way for ~100% of the time.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>> -Wswitch-default
>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>> to follow
>>>>>>>>> the existing "error handling" at those places.
>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>> continue to spread it.
>>>>>>>>
>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>> addressed
>>>>>>>> So we should not add more of them...
>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>> it is
>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>> is best to try to address partially the problem directly then having so you
>>>>>> reduce the amounts of change afterwards.
>>>>>>
>>>>>> So please try to not introduce more BUG() in the code base.
>>>>> Hi Oleksandr, Julien,
>>>>>
>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>> more (not less!) resilient to failure.
>>>>>
>>>>> So, I think it is a good idea to introduce a default label because it
>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>> default handler, which is detrimental, we should return an error when
>>>>> possible, or just print a warning.
>>>>
>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>> if it gets hit, and wont crash Xen.
>>>
>>> That's a good suggestion.
>>>
>>>
>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>> the rule. We just need to explain why a default label is not needed.
>>>>> Such as:
>>>>>
>>>>>     default:
>>>>>     /* unreachable because blah and blah */
>>>>
>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>>
>> Well, you could build with GCC and then build with your custom
>> compiler...
> 
> This suggestion is problematic: as an individual interested in MISRA-C
> compliance, I only have the MISRA-C rules in my hands. I don't know how
> to deal with suggestions like this one, that don't comply to the Rules,
> but it tries to address the same issue in a different manner.

Are you suggesting we will have to abide to all the rules even if they 
doesn't make things worst? I was under the impression we don't necessary 
need to follow a rule if we have justification for it.

> 
> I cannot rule out that it wouldn't work, but also I cannot be sure that
> it would work. In short, I have no way to make progress or to find out
> how to move forward. I guess as a contributor I would be forced to go
> back to the MISRAC compliance experts and ask for their opinion. (One
> non-technical issue is who is going to pay them for spending their time
> on this.) But what if they say it is not acceptable for compliance?
> 
> This is a great topic to discuss in March and decide what to do in these
> situations.
> 
> 
>> But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> Obviously, this has to change if we want to make progress on safety
> certifications.

I am curious to know what is plan for this. I mean that if no-one is 
planning to make Xen build with other compilers. Then what would be the 
benefits of this?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>> -Wswitch-default
>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>> to follow
>>>>>>>>> the existing "error handling" at those places.
>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>> continue to spread it.
>>>>>>>>
>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>> addressed
>>>>>>>> So we should not add more of them...
>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>> it is
>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>> is best to try to address partially the problem directly then having so you
>>>>>> reduce the amounts of change afterwards.
>>>>>>
>>>>>> So please try to not introduce more BUG() in the code base.
>>>>> Hi Oleksandr, Julien,
>>>>>
>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>> more (not less!) resilient to failure.
>>>>>
>>>>> So, I think it is a good idea to introduce a default label because it
>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>> default handler, which is detrimental, we should return an error when
>>>>> possible, or just print a warning.
>>>>
>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>> if it gets hit, and wont crash Xen.
>>>
>>> That's a good suggestion.
>>>
>>>
>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>> the rule. We just need to explain why a default label is not needed.
>>>>> Such as:
>>>>>
>>>>>     default:
>>>>>     /* unreachable because blah and blah */
>>>>
>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>>
>> Well, you could build with GCC and then build with your custom
>> compiler...
> 
> This suggestion is problematic: as an individual interested in MISRA-C
> compliance, I only have the MISRA-C rules in my hands. I don't know how
> to deal with suggestions like this one, that don't comply to the Rules,
> but it tries to address the same issue in a different manner.

Are you suggesting we will have to abide to all the rules even if they 
doesn't make things worst? I was under the impression we don't necessary 
need to follow a rule if we have justification for it.

> 
> I cannot rule out that it wouldn't work, but also I cannot be sure that
> it would work. In short, I have no way to make progress or to find out
> how to move forward. I guess as a contributor I would be forced to go
> back to the MISRAC compliance experts and ask for their opinion. (One
> non-technical issue is who is going to pay them for spending their time
> on this.) But what if they say it is not acceptable for compliance?

I appreciate people might want to use Xen with MISRA C. But I am not 
convinced we should bend to some rules in Xen Project for the sake of 
making MISRA happy. People could carry such patch themselves if they 
care about it.

> 
> This is a great topic to discuss in March and decide what to do in these
> situations.
> 
> 
>> But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> Obviously, this has to change if we want to make progress on safety
> certifications.

I am curious to know what is plan for this. I mean that if no-one is 
planning to make Xen build with other compilers. Then what would be the 
benefits of this?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:34, Andrew Cooper wrote:
> On 22/02/2019 22:11, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>> -Wswitch-default
>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>> to follow
>>>>>>>>> the existing "error handling" at those places.
>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>> continue to spread it.
>>>>>>>>
>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>> addressed
>>>>>>>> So we should not add more of them...
>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>> it is
>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>> is best to try to address partially the problem directly then having so you
>>>>>> reduce the amounts of change afterwards.
>>>>>>
>>>>>> So please try to not introduce more BUG() in the code base.
>>>>> Hi Oleksandr, Julien,
>>>>>
>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>> more (not less!) resilient to failure.
>>>>>
>>>>> So, I think it is a good idea to introduce a default label because it
>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>> default handler, which is detrimental, we should return an error when
>>>>> possible, or just print a warning.
>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>> if it gets hit, and wont crash Xen.
>>> That's a good suggestion.
>>>
>>>
>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>> the rule. We just need to explain why a default label is not needed.
>>>>> Such as:
>>>>>
>>>>>     default:
>>>>>     /* unreachable because blah and blah */
>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>> compiler specific requirement. Typically gcc is not the compiler of
>>> choice for these environments, unfortunately forcing gcc is not an
>>> option.
>> Well, you could build with GCC and then build with your custom
>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>> as we don't build with clang and I pretty doubt we can build with compcert.
> 
> So the suggestion I had was to have an overall CONFIG_MISRA which we can
> hide some of this nonsense behind, and then
> 
> #ifdef CONFIG_MISRA
> #define MISRA_BLE_DEFAULT default:
> #else
> #define MISRA_BLE_DEFAULT
> #endif

This is pretty disgusting :). But then, it makes the code is bit more 
obscure. So how that rule is making Xen more safe? Furthermore, one 
default may not rule them all. So aren't we just adding code to make 
MISRA happy at the risk of introducing more bug?

>
> So when you disable CONFIG_MISRA, your compiler starts being able to
> help you again.
> 
> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
> continue doing it the efficient way for ~100% of the time.

My main concern is we only looked at 2 MISRA rules and we already need 
some nonsense code to spread everywhere. How much more are we going to 
get like that?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 22:38, Stefano Stabellini wrote:
> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>> On 22/02/2019 22:11, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>> -Wswitch-default
>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>> to follow
>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>> continue to spread it.
>>>>>>>>>
>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>> addressed
>>>>>>>>> So we should not add more of them...
>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>> it is
>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>> reduce the amounts of change afterwards.
>>>>>>>
>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>> Hi Oleksandr, Julien,
>>>>>>
>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>> more (not less!) resilient to failure.
>>>>>>
>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>> default handler, which is detrimental, we should return an error when
>>>>>> possible, or just print a warning.
>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>> if it gets hit, and wont crash Xen.
>>>> That's a good suggestion.
>>>>
>>>>
>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>> Such as:
>>>>>>
>>>>>>     default:
>>>>>>     /* unreachable because blah and blah */
>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>> choice for these environments, unfortunately forcing gcc is not an
>>>> option.
>>> Well, you could build with GCC and then build with your custom
>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>> as we don't build with clang and I pretty doubt we can build with compcert.
>>
>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>> hide some of this nonsense behind, and then
>>
>> #ifdef CONFIG_MISRA
>> #define MISRA_BLE_DEFAULT default:
>> #else
>> #define MISRA_BLE_DEFAULT
>> #endif
>>
>> So when you disable CONFIG_MISRA, your compiler starts being able to
>> help you again.
> 
> This is actually a good way to make progress which could make everybody
> happy. (And it wouldn't require a slow back-and-forth with third parties
> to ask difficult questions.)

I can tell you I am not happy with that :). We would make the code more 
obscure. So it raises question on what would be the benefits of adopting 
the rule in Xen.

But maybe the first question is how much we need to adhere to those 
rules. What are the consequences of not following them in Xen Project? I 
know that some upstream project chose to not apply to all the rules.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Andrew Cooper]

On 22/02/2019 23:22, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 22:34, Andrew Cooper wrote:
>> On 22/02/2019 22:11, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>> -Wswitch-default
>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>> to follow
>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>> continue to spread it.
>>>>>>>>>
>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>> addressed
>>>>>>>>> So we should not add more of them...
>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>> it is
>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>> reduce the amounts of change afterwards.
>>>>>>>
>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>> Hi Oleksandr, Julien,
>>>>>>
>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>> more (not less!) resilient to failure.
>>>>>>
>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>> default handler, which is detrimental, we should return an error when
>>>>>> possible, or just print a warning.
>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>> if it gets hit, and wont crash Xen.
>>>> That's a good suggestion.
>>>>
>>>>
>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>> Such as:
>>>>>>
>>>>>>     default:
>>>>>>     /* unreachable because blah and blah */
>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>> choice for these environments, unfortunately forcing gcc is not an
>>>> option.
>>> Well, you could build with GCC and then build with your custom
>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>> as we don't build with clang and I pretty doubt we can build with compcert.
>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>> hide some of this nonsense behind, and then
>>
>> #ifdef CONFIG_MISRA
>> #define MISRA_BLE_DEFAULT default:
>> #else
>> #define MISRA_BLE_DEFAULT
>> #endif
> This is pretty disgusting :). But then, it makes the code is bit more 
> obscure. So how that rule is making Xen more safe? Furthermore, one 
> default may not rule them all. So aren't we just adding code to make 
> MISRA happy at the risk of introducing more bug?

Any switch statement where the default isn't empty
(assert/bug/domain_crash/print) should probably be a regular default:

I very much doubt MISRA intended for people to make themselves compliant
by adding "default: break;" everywhere, but lethargy is a powerful
driving force.

An alternative, and substantially more ugly would be to have the else
case be "if ( 0 )" or similar, so you do take out the full next block,
but that requires people to member braces.

Even more ugly would be to take (x) and construct the block manually,
but then you get at minimum a set of brackets where you would expect to
see braces in normal C, and this alone is confusing to read.

>> So when you disable CONFIG_MISRA, your compiler starts being able to
>> help you again.
>>
>> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
>> continue doing it the efficient way for ~100% of the time.
> My main concern is we only looked at 2 MISRA rules and we already need 
> some nonsense code to spread everywhere. How much more are we going to 
> get like that?

MISRA scanning is just like Coverity scanning.  The item classes with
the highest seen-count tend to be the least interesting and relevant, as
they tend to be the ones where we are systematically different.

It is the one-off flagged issues which tend to be important.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/22/19 11:33 PM, Andrew Cooper wrote:
> On 22/02/2019 21:00, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>> -Wswitch-default
>>>>>> Warn whenever a switch statement does not have a default case.
Will you be ok to turn this particular switch on by default?
Or you suggest that anyone interested to do so on their own will?
If we want to turn many more warnings a bit later?
>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>> to follow
>>>>>> the existing "error handling" at those places.
>>>>> It is not because BUG() is been used today in some places that we need to
>>>>> continue to spread it.
>>>>>
>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>> addressed
>>>>> So we should not add more of them...
>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>> it is
>>>> acceptable to use the existing way of error handling if any at all.
>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>> is best to try to address partially the problem directly then having so you
>>> reduce the amounts of change afterwards.
So, then we need to get rid of BUG() from the existing code first,
not trying to solve two issues at a time: rule 16.4 and BUG().
I still do think these two changes are not strictly relevant,
but 16.4 just suffers from consequences of BUG() being used.
>>>
>>> So please try to not introduce more BUG() in the code base.
>> Hi Oleksandr, Julien,
>>
>> Julien's right that we should not introduce any more BUG()s. In fact,
>> each of them makes the code less safe, not more safe! The purpose of
>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>> more (not less!) resilient to failure.
>>
>> So, I think it is a good idea to introduce a default label because it
>> can help us spot unexpected issues. Instead of calling BUG() in the
>> default handler, which is detrimental, we should return an error when
>> possible, or just print a warning.
> domain_crash() is almost always better than BUG().  It is very obvious
> if it gets hit, and wont crash Xen.
Thank you for suggestions
>> As 16.4 clearly state, even a simple comment would be enough to address
>> the rule. We just need to explain why a default label is not needed.
>> Such as:
>>
>>    default:
>>    /* unreachable because blah and blah */
This is true...
> What a simple comment doesn't do is avoid breaking -Wswitch.
... as well as this comment as well.
>
> This requirement is actively hostile towards compilers trying to help
> you spot when you made a mistake and forgot to update one of the $N
> places you needed to.
This is a trade-off: if your compiler is buggy or not supporting
the switches then the code itself defends from such cases, hence
the "default".
>
> In this case, I don't think "Because MISRA demand it" is a good enough
> justification to offset the increased error-prone-ness of the result.
...and if you forget to enable -Wswitch or use a compiler that
doesn't support this switch then what?
> ~Andrew
>
> P.S. There is a solution here which could work, but IMO a better use of
> time and energy would be to get MISRA to update their rules to match
> this century, and stop getting in the way of compiler features intended
> to help the programmer avoid bugs.
Well, I am a little bit pessimistic here. The same suggestion
MISRA guys may have: go change your code as the rules we have
are widely accepted and the code which wants to comply must obey the
rules. What is more is that if MISRA agrees that compiler switches can
be used, then what are those compilers? I can imagine that various versions
of even gcc might not support all the switches we want. So what happens
next? We dictate gcc guys to make sure to support all the switches that we
agreed with MISRA?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/23/19 1:34 AM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 22:38, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>> On 22/02/2019 22:11, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>> to follow
>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>>> continue to spread it.
>>>>>>>>>>
>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>> addressed
>>>>>>>>>> So we should not add more of them...
>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>>> it is
>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>>
>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>> Hi Oleksandr, Julien,
>>>>>>>
>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>> more (not less!) resilient to failure.
>>>>>>>
>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>> possible, or just print a warning.
>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>> if it gets hit, and wont crash Xen.
>>>>> That's a good suggestion.
>>>>>
>>>>>
>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>> Such as:
>>>>>>>
>>>>>>>      default:
>>>>>>>      /* unreachable because blah and blah */
>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>>> choice for these environments, unfortunately forcing gcc is not an
>>>>> option.
>>>> Well, you could build with GCC and then build with your custom
>>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>>> as we don't build with clang and I pretty doubt we can build with compcert.
>>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>>> hide some of this nonsense behind, and then
>>>
>>> #ifdef CONFIG_MISRA
>>> #define MISRA_BLE_DEFAULT default:
>>> #else
>>> #define MISRA_BLE_DEFAULT
>>> #endif
>>>
>>> So when you disable CONFIG_MISRA, your compiler starts being able to
>>> help you again.
>> This is actually a good way to make progress which could make everybody
>> happy. (And it wouldn't require a slow back-and-forth with third parties
>> to ask difficult questions.)
> I can tell you I am not happy with that :). We would make the code more
> obscure. So it raises question on what would be the benefits of adopting
> the rule in Xen.
And this is the very first change which leads to those CONFIG_MISRA
things, but what happens next when we want to add even more of those?
I do agree here that code will become just a huge amount of #ifdef's
at the end of the day.
> But maybe the first question is how much we need to adhere to those
> rules.
I am no way an expert in certification, but from my POV you either
comply or not. There is no solution in between. As per my understanding
MISRA allows some documented exceptions, but I'm just curious if they
expect most of the code to have those exceptions...
Experts, please step in...
>   What are the consequences of not following them in Xen Project? I
> know that some upstream project chose to not apply to all the rules.
Not all of the upstream projects want to be certified...
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/23/19 1:41 AM, Andrew Cooper wrote:
> On 22/02/2019 23:22, Julien Grall wrote:
>> Hi,
>>
>> On 22/02/2019 22:34, Andrew Cooper wrote:
>>> On 22/02/2019 22:11, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>> to follow
>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>>> continue to spread it.
>>>>>>>>>>
>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>> addressed
>>>>>>>>>> So we should not add more of them...
>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>>> it is
>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>>
>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>> Hi Oleksandr, Julien,
>>>>>>>
>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>> more (not less!) resilient to failure.
>>>>>>>
>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>> possible, or just print a warning.
>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>> if it gets hit, and wont crash Xen.
>>>>> That's a good suggestion.
>>>>>
>>>>>
>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>> Such as:
>>>>>>>
>>>>>>>      default:
>>>>>>>      /* unreachable because blah and blah */
>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>>> choice for these environments, unfortunately forcing gcc is not an
>>>>> option.
>>>> Well, you could build with GCC and then build with your custom
>>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>>> as we don't build with clang and I pretty doubt we can build with compcert.
>>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>>> hide some of this nonsense behind, and then
>>>
>>> #ifdef CONFIG_MISRA
>>> #define MISRA_BLE_DEFAULT default:
>>> #else
>>> #define MISRA_BLE_DEFAULT
>>> #endif
>> This is pretty disgusting :). But then, it makes the code is bit more
>> obscure. So how that rule is making Xen more safe? Furthermore, one
>> default may not rule them all. So aren't we just adding code to make
>> MISRA happy at the risk of introducing more bug?
> Any switch statement where the default isn't empty
> (assert/bug/domain_crash/print) should probably be a regular default:
>
> I very much doubt MISRA intended for people to make themselves compliant
> by adding "default: break;" everywhere, but lethargy is a powerful
> driving force.
>
> An alternative, and substantially more ugly would be to have the else
> case be "if ( 0 )" or similar, so you do take out the full next block,
> but that requires people to member braces.
>
> Even more ugly would be to take (x) and construct the block manually,
> but then you get at minimum a set of brackets where you would expect to
> see braces in normal C, and this alone is confusing to read.
>
>>> So when you disable CONFIG_MISRA, your compiler starts being able to
>>> help you again.
>>>
>>> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
>>> continue doing it the efficient way for ~100% of the time.
>> My main concern is we only looked at 2 MISRA rules and we already need
>> some nonsense code to spread everywhere. How much more are we going to
>> get like that?
(Un)fortunately many more...
> MISRA scanning is just like Coverity scanning.  The item classes with
> the highest seen-count tend to be the least interesting and relevant, as
> they tend to be the ones where we are systematically different.
>
> It is the one-off flagged issues which tend to be important.
I do agree. There are many more more serious issues on the list.
The "default" rule is just one to try and seemed very straight forward
to fix (I put aside BUG() issue - this was expected to be discussed)
> ~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

[-- Attachment #1: Type: text/html, Size: 4598 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/23/19 1:13 AM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 22:34, Stefano Stabellini wrote:
>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>> -Wswitch-default
>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>> to follow
>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>> continue to spread it.
>>>>>>>>>
>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>> addressed
>>>>>>>>> So we should not add more of them...
>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>> it is
>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>> reduce the amounts of change afterwards.
>>>>>>>
>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>> Hi Oleksandr, Julien,
>>>>>>
>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>> more (not less!) resilient to failure.
>>>>>>
>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>> default handler, which is detrimental, we should return an error when
>>>>>> possible, or just print a warning.
>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>> if it gets hit, and wont crash Xen.
>>>> That's a good suggestion.
>>>>
>>>>
>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>> Such as:
>>>>>>
>>>>>>      default:
>>>>>>      /* unreachable because blah and blah */
>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>> choice for these environments, unfortunately forcing gcc is not an
>>>> option.
>>> Well, you could build with GCC and then build with your custom
>>> compiler...
>> This suggestion is problematic: as an individual interested in MISRA-C
>> compliance, I only have the MISRA-C rules in my hands. I don't know how
>> to deal with suggestions like this one, that don't comply to the Rules,
>> but it tries to address the same issue in a different manner.
> Are you suggesting we will have to abide to all the rules even if they
> doesn't make things worst? I was under the impression we don't necessary
> need to follow a rule if we have justification for it.
>
>> I cannot rule out that it wouldn't work, but also I cannot be sure that
>> it would work. In short, I have no way to make progress or to find out
>> how to move forward. I guess as a contributor I would be forced to go
>> back to the MISRAC compliance experts and ask for their opinion. (One
>> non-technical issue is who is going to pay them for spending their time
>> on this.) But what if they say it is not acceptable for compliance?
> I appreciate people might want to use Xen with MISRA C. But I am not
> convinced we should bend to some rules in Xen Project for the sake of
> making MISRA happy. People could carry such patch themselves if they
> care about it.
Good suggestion, but taking into account the number of possible
changes this will finally become a fork of the existing Xen...
And we all probably understand what it means to deviate from upstream...
On the other hand what's wrong with the will to make the project
benefit from more robust (as MISRA believes) and, yes, certified code?
>
>> This is a great topic to discuss in March and decide what to do in these
>> situations.
>>
>>
>>> But, GCC is pretty much the only choice for Xen on Arm today
>>> as we don't build with clang and I pretty doubt we can build with compcert.
>> Obviously, this has to change if we want to make progress on safety
>> certifications.
> I am curious to know what is plan for this. I mean that if no-one is
> planning to make Xen build with other compilers. Then what would be the
> benefits of this?
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 10:27, Oleksandr Andrushchenko wrote:
> On 2/23/19 1:13 AM, Julien Grall wrote:
>> Hi,
>>
>> On 22/02/2019 22:34, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>> to follow
>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>>> continue to spread it.
>>>>>>>>>>
>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>> addressed
>>>>>>>>>> So we should not add more of them...
>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>>> it is
>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>> That's not how it works in upstream. If you know some constructs are 
>>>>>>>> wrong, it
>>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>>
>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>> Hi Oleksandr, Julien,
>>>>>>>
>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>> more (not less!) resilient to failure.
>>>>>>>
>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>> possible, or just print a warning.
>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>> if it gets hit, and wont crash Xen.
>>>>> That's a good suggestion.
>>>>>
>>>>>
>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>> Such as:
>>>>>>>
>>>>>>>      default:
>>>>>>>      /* unreachable because blah and blah */
>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>>> choice for these environments, unfortunately forcing gcc is not an
>>>>> option.
>>>> Well, you could build with GCC and then build with your custom
>>>> compiler...
>>> This suggestion is problematic: as an individual interested in MISRA-C
>>> compliance, I only have the MISRA-C rules in my hands. I don't know how
>>> to deal with suggestions like this one, that don't comply to the Rules,
>>> but it tries to address the same issue in a different manner.
>> Are you suggesting we will have to abide to all the rules even if they
>> doesn't make things worst? I was under the impression we don't necessary
>> need to follow a rule if we have justification for it.
>>
>>> I cannot rule out that it wouldn't work, but also I cannot be sure that
>>> it would work. In short, I have no way to make progress or to find out
>>> how to move forward. I guess as a contributor I would be forced to go
>>> back to the MISRAC compliance experts and ask for their opinion. (One
>>> non-technical issue is who is going to pay them for spending their time
>>> on this.) But what if they say it is not acceptable for compliance?
>> I appreciate people might want to use Xen with MISRA C. But I am not
>> convinced we should bend to some rules in Xen Project for the sake of
>> making MISRA happy. People could carry such patch themselves if they
>> care about it.
> Good suggestion, but taking into account the number of possible
> changes this will finally become a fork of the existing Xen...
> And we all probably understand what it means to deviate from upstream...
> On the other hand what's wrong with the will to make the project
> benefit from more robust (as MISRA believes) and, yes, certified code?
 From my understanding, the goal is to have Xen upstream close to be compliant 
with MISRA. Then the code can be consumed by company (such as yours) to fully 
certify Xen. The discussion here is to understand what are the pros/cons on 
having this rule fully implemented in Xen.

While I can see the benefits in some of the switch, I doubt this will help in 
others switch (see my answer on patch #1). I am still waiting to see a 
justification on why MISRA believes our code will be more robust with "default: 
break;" code...

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Oleksandr,

On 25/02/2019 10:00, Oleksandr Andrushchenko wrote:
> On 2/23/19 1:34 AM, Julien Grall wrote:
>>   What are the consequences of not following them in Xen Project? I
>> know that some upstream project chose to not apply to all the rules.
> Not all of the upstream projects want to be certified...

You missed my point... I was talking about upstream project aiming to be MISRA C 
compliant.

The thing is upstream project is unlikely to be used as is by company targeting 
certification. They will have to modify the code to meet their requirements.

Where upstream can help is adopting some of the rules so the company does not 
have to carry a lot patches on top. As maintainer, I have to weight the 
pros/cons of accepting those patches in Xen.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 10:06, Oleksandr Andrushchenko wrote:
> On 2/23/19 1:41 AM, Andrew Cooper wrote:
>> On 22/02/2019 23:22, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/02/2019 22:34, Andrew Cooper wrote:
>>>> On 22/02/2019 22:11, Julien Grall wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>>> to follow
>>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>>> It is not because BUG() is been used today in some places that we 
>>>>>>>>>>> need to
>>>>>>>>>>> continue to spread it.
>>>>>>>>>>>
>>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>>> addressed
>>>>>>>>>>> So we should not add more of them...
>>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I 
>>>>>>>>>> think
>>>>>>>>>> it is
>>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>>> That's not how it works in upstream. If you know some constructs are 
>>>>>>>>> wrong, it
>>>>>>>>> is best to try to address partially the problem directly then having so 
>>>>>>>>> you
>>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>>>
>>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>>> Hi Oleksandr, Julien,
>>>>>>>>
>>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>>> more (not less!) resilient to failure.
>>>>>>>>
>>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>>> possible, or just print a warning.
>>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>>> if it gets hit, and wont crash Xen.
>>>>>> That's a good suggestion.
>>>>>>
>>>>>>
>>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>>> Such as:
>>>>>>>>
>>>>>>>>      default:
>>>>>>>>      /* unreachable because blah and blah */
>>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
>>>>>> -Wswitch could be a good way to address 16.4, but then we introduce a
>>>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>>>> choice for these environments, unfortunately forcing gcc is not an
>>>>>> option.
>>>>> Well, you could build with GCC and then build with your custom
>>>>> compiler... But, GCC is pretty much the only choice for Xen on Arm today
>>>>> as we don't build with clang and I pretty doubt we can build with compcert.
>>>> So the suggestion I had was to have an overall CONFIG_MISRA which we can
>>>> hide some of this nonsense behind, and then
>>>>
>>>> #ifdef CONFIG_MISRA
>>>> #define MISRA_BLE_DEFAULT default:
>>>> #else
>>>> #define MISRA_BLE_DEFAULT
>>>> #endif
>>> This is pretty disgusting :). But then, it makes the code is bit more
>>> obscure. So how that rule is making Xen more safe? Furthermore, one
>>> default may not rule them all. So aren't we just adding code to make
>>> MISRA happy at the risk of introducing more bug?
>> Any switch statement where the default isn't empty
>> (assert/bug/domain_crash/print) should probably be a regular default:
>>
>> I very much doubt MISRA intended for people to make themselves compliant
>> by adding "default: break;" everywhere, but lethargy is a powerful
>> driving force.
>>
>> An alternative, and substantially more ugly would be to have the else
>> case be "if ( 0 )" or similar, so you do take out the full next block,
>> but that requires people to member braces.
>>
>> Even more ugly would be to take (x) and construct the block manually,
>> but then you get at minimum a set of brackets where you would expect to
>> see braces in normal C, and this alone is confusing to read.
>>
>>>> So when you disable CONFIG_MISRA, your compiler starts being able to
>>>> help you again.
>>>>
>>>> TBH, it would also be nice to hide the SYMBOL nonsense behind, so we can
>>>> continue doing it the efficient way for ~100% of the time.
>>> My main concern is we only looked at 2 MISRA rules and we already need
>>> some nonsense code to spread everywhere. How much more are we going to
>>> get like that?
> (Un)fortunately many more...

Do you mind providing a list of MISRA C rule that will require tree-wide changes?

>> MISRA scanning is just like Coverity scanning.  The item classes with
>> the highest seen-count tend to be the least interesting and relevant, as
>> they tend to be the ones where we are systematically different.
>>
>> It is the one-off flagged issues which tend to be important.
> I do agree. There are many more more serious issues on the list.
> The "default" rule is just one to try and seemed very straight forward
> to fix (I put aside BUG() issue - this was expected to be discussed)

It feels to me we should try to address serious issues first and then look at 
more controversial changes afterwards.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>> -Wswitch-default
>>>>>>> Warn whenever a switch statement does not have a default case.
> Will you be ok to turn this particular switch on by default?
> Or you suggest that anyone interested to do so on their own will?
> If we want to turn many more warnings a bit later?

If we decide to fully implement the rule in Xen, then it would be nice to get 
the compiler helping us spotting new switch without default case.

>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>> to follow
>>>>>>> the existing "error handling" at those places.
>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>> continue to spread it.
>>>>>>
>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>> addressed
>>>>>> So we should not add more of them...
>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>> it is
>>>>> acceptable to use the existing way of error handling if any at all.
>>>> That's not how it works in upstream. If you know some constructs are wrong, it
>>>> is best to try to address partially the problem directly then having so you
>>>> reduce the amounts of change afterwards.
> So, then we need to get rid of BUG() from the existing code first,
> not trying to solve two issues at a time: rule 16.4 and BUG().
> I still do think these two changes are not strictly relevant,
> but 16.4 just suffers from consequences of BUG() being used.

Why would you add more BUG()? As I said above if this is an issue, then you 
don't add more. We can deal with other later on.

So let me be clearer, I will not accept any new defensive code using BUG(). If 
you need rework in order to avoid BUG() in your series, then please do it. Note, 
this is not a request to remove the existing BUG().

>>>>
>>>> So please try to not introduce more BUG() in the code base.
>>> Hi Oleksandr, Julien,
>>>
>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>> each of them makes the code less safe, not more safe! The purpose of
>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>> more (not less!) resilient to failure.
>>>
>>> So, I think it is a good idea to introduce a default label because it
>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>> default handler, which is detrimental, we should return an error when
>>> possible, or just print a warning.
>> domain_crash() is almost always better than BUG().  It is very obvious
>> if it gets hit, and wont crash Xen.
> Thank you for suggestions
>>> As 16.4 clearly state, even a simple comment would be enough to address
>>> the rule. We just need to explain why a default label is not needed.
>>> Such as:
>>>
>>>    default:
>>>    /* unreachable because blah and blah */
> This is true...
>> What a simple comment doesn't do is avoid breaking -Wswitch.
> ... as well as this comment as well.
>>
>> This requirement is actively hostile towards compilers trying to help
>> you spot when you made a mistake and forgot to update one of the $N
>> places you needed to.
> This is a trade-off: if your compiler is buggy or not supporting
> the switches then the code itself defends from such cases, hence
> the "default".

As said before, how do you put the limit on what you need to protect against? If 
your compiler is buggy, then probably a default in switch is not going to help much.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 1:08 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> On 25/02/2019 10:00, Oleksandr Andrushchenko wrote:
>> On 2/23/19 1:34 AM, Julien Grall wrote:
>>>   What are the consequences of not following them in Xen Project? I
>>> know that some upstream project chose to not apply to all the rules.
>> Not all of the upstream projects want to be certified...
>
> You missed my point... I was talking about upstream project aiming to 
> be MISRA C compliant.
>
Not sure if I really missed it
> The thing is upstream project is unlikely to be used as is by company 
> targeting certification. They will have to modify the code to meet 
> their requirements.
>
> Where upstream can help is adopting some of the rules so the company 
> does not have to carry a lot patches on top. As maintainer, I have to 
> weight the pros/cons of accepting those patches in Xen.
>
I do understand your concerns
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 22.02.19 at 22:33, <andrew.cooper3@citrix.com> wrote:
> P.S. There is a solution here which could work, but IMO a better use of
> time and energy would be to get MISRA to update their rules to match
> this century, and stop getting in the way of compiler features intended
> to help the programmer avoid bugs.

As much as I'm with you in desiring the compiler aid given to not get
undermined, I think this MISRA rule isn't in need of modernizing: It's
one thing for the compiler to help with in-range enumerators, and it's
another to demand that unintentional out-of-range ones don't cause
actual harm (like crashing your car into the next tree). This is even
more so that iirc there's no warning if you pass a plain integer into a
function whose parameter specifies an enum, or if you assign a plain
integer to an enum types variable.

Amongst our planned hardening Kconfig-ery I think we may want to
add an option controlling whether to have something like

#define DEFAULT_UNREACHABLE(code) default: ASSERT_UNREACHABLE(); code;

or instead (by default) expanding to nothing.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 22/02/2019 10:27, Andrew Cooper wrote:
> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Hello, everybody!
>>
>> We at EPAM Systems would like to present first series of patches targeting Xen
>> on ARM Functional Safety certification (ISO61508 based): implementation of
>> MISRA [1] C:2012 Rule 16.4 which requires that every switch statement has a
>> default label as a measure of defensive programming technique.
> 
> Hang on - what?
> 
> Can someone attempt to justify why actively breaking -Wswitch is going
> to result in safer/better code?

I just discovered that GCC has an option -Wswitch-enum that would warn of 
missing element even in the presence of the 'default' label. I guess this could 
help us to reunite MISRA and Xen for this rule?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 1:10 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 10:06, Oleksandr Andrushchenko wrote:
>> On 2/23/19 1:41 AM, Andrew Cooper wrote:
>>> On 22/02/2019 23:22, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/02/2019 22:34, Andrew Cooper wrote:
>>>>> On 22/02/2019 22:11, Julien Grall wrote:
>>>>>> Hi Stefano,
>>>>>>
>>>>>> On 22/02/2019 21:58, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
>>>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>>>> Warn whenever a switch statement does not have a default 
>>>>>>>>>>>>> case.
>>>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is 
>>>>>>>>>>>>> that I tried
>>>>>>>>>>>>> to follow
>>>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>>>> It is not because BUG() is been used today in some places 
>>>>>>>>>>>> that we need to
>>>>>>>>>>>> continue to spread it.
>>>>>>>>>>>>
>>>>>>>>>>>>> Use of BUG() itself is another topic which will also need 
>>>>>>>>>>>>> to be
>>>>>>>>>>>>> addressed
>>>>>>>>>>>> So we should not add more of them...
>>>>>>>>>>> Again, I see this as a dedicated change. So, in the current 
>>>>>>>>>>> series I think
>>>>>>>>>>> it is
>>>>>>>>>>> acceptable to use the existing way of error handling if any 
>>>>>>>>>>> at all.
>>>>>>>>>> That's not how it works in upstream. If you know some 
>>>>>>>>>> constructs are wrong, it
>>>>>>>>>> is best to try to address partially the problem directly then 
>>>>>>>>>> having so you
>>>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>>>>
>>>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>>>> Hi Oleksandr, Julien,
>>>>>>>>>
>>>>>>>>> Julien's right that we should not introduce any more BUG()s. 
>>>>>>>>> In fact,
>>>>>>>>> each of them makes the code less safe, not more safe! The 
>>>>>>>>> purpose of
>>>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a 
>>>>>>>>> way that is
>>>>>>>>> more (not less!) resilient to failure.
>>>>>>>>>
>>>>>>>>> So, I think it is a good idea to introduce a default label 
>>>>>>>>> because it
>>>>>>>>> can help us spot unexpected issues. Instead of calling BUG() 
>>>>>>>>> in the
>>>>>>>>> default handler, which is detrimental, we should return an 
>>>>>>>>> error when
>>>>>>>>> possible, or just print a warning.
>>>>>>>> domain_crash() is almost always better than BUG(). It is very 
>>>>>>>> obvious
>>>>>>>> if it gets hit, and wont crash Xen.
>>>>>>> That's a good suggestion.
>>>>>>>
>>>>>>>
>>>>>>>>> As 16.4 clearly state, even a simple comment would be enough 
>>>>>>>>> to address
>>>>>>>>> the rule. We just need to explain why a default label is not 
>>>>>>>>> needed.
>>>>>>>>> Such as:
>>>>>>>>>
>>>>>>>>>      default:
>>>>>>>>>      /* unreachable because blah and blah */
>>>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>>>> I don't know how to reconcile 16.4 with -Wswitch. One could 
>>>>>>> argue that
>>>>>>> -Wswitch could be a good way to address 16.4, but then we 
>>>>>>> introduce a
>>>>>>> compiler specific requirement. Typically gcc is not the compiler of
>>>>>>> choice for these environments, unfortunately forcing gcc is not an
>>>>>>> option.
>>>>>> Well, you could build with GCC and then build with your custom
>>>>>> compiler... But, GCC is pretty much the only choice for Xen on 
>>>>>> Arm today
>>>>>> as we don't build with clang and I pretty doubt we can build with 
>>>>>> compcert.
>>>>> So the suggestion I had was to have an overall CONFIG_MISRA which 
>>>>> we can
>>>>> hide some of this nonsense behind, and then
>>>>>
>>>>> #ifdef CONFIG_MISRA
>>>>> #define MISRA_BLE_DEFAULT default:
>>>>> #else
>>>>> #define MISRA_BLE_DEFAULT
>>>>> #endif
>>>> This is pretty disgusting :). But then, it makes the code is bit more
>>>> obscure. So how that rule is making Xen more safe? Furthermore, one
>>>> default may not rule them all. So aren't we just adding code to make
>>>> MISRA happy at the risk of introducing more bug?
>>> Any switch statement where the default isn't empty
>>> (assert/bug/domain_crash/print) should probably be a regular default:
>>>
>>> I very much doubt MISRA intended for people to make themselves 
>>> compliant
>>> by adding "default: break;" everywhere, but lethargy is a powerful
>>> driving force.
>>>
>>> An alternative, and substantially more ugly would be to have the else
>>> case be "if ( 0 )" or similar, so you do take out the full next block,
>>> but that requires people to member braces.
>>>
>>> Even more ugly would be to take (x) and construct the block manually,
>>> but then you get at minimum a set of brackets where you would expect to
>>> see braces in normal C, and this alone is confusing to read.
>>>
>>>>> So when you disable CONFIG_MISRA, your compiler starts being able to
>>>>> help you again.
>>>>>
>>>>> TBH, it would also be nice to hide the SYMBOL nonsense behind, so 
>>>>> we can
>>>>> continue doing it the efficient way for ~100% of the time.
>>>> My main concern is we only looked at 2 MISRA rules and we already need
>>>> some nonsense code to spread everywhere. How much more are we going to
>>>> get like that?
>> (Un)fortunately many more...
>
> Do you mind providing a list of MISRA C rule that will require 
> tree-wide changes?
>
I am not quite sure anyone else or I can do this at the moment
>>> MISRA scanning is just like Coverity scanning.  The item classes with
>>> the highest seen-count tend to be the least interesting and 
>>> relevant, as
>>> they tend to be the ones where we are systematically different.
>>>
>>> It is the one-off flagged issues which tend to be important.
>> I do agree. There are many more more serious issues on the list.
>> The "default" rule is just one to try and seemed very straight forward
>> to fix (I put aside BUG() issue - this was expected to be discussed)
>
> It feels to me we should try to address serious issues first and then 
> look at more controversial changes afterwards.
>
While I do agree in common with this approach the reason for bringing in
some "controversial changes" was our hope to understand community's feeling
about that in general and try to work out an accepted way for such changes.

> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 10:11, Oleksandr Andrushchenko wrote:
> On 2/23/19 12:08 AM, Julien Grall wrote:
>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>> -Wswitch-default
>>> As 16.4 clearly state, even a simple comment would be enough to address
>>> the rule. We just need to explain why a default label is not needed.
>>> Such as:
>>>
>>>     default:
>>>     /* unreachable because blah and blah */
>> ... as we already have defensive code. Indeed, in most of the switch we
>> deal with potential issue by initializing the variable before the
>> switch. If you look at the first patch, a lot of "default: break;" is
>> introduced. So what's our benefits? How this code is more defensive than
>> what we currently have?
> ...and if you remove those initialized variables and forget
> to update the switch with default? Or change the initial value
> the way your previous switch assumptions do not work anymore?
> I am not trying to defend MISRA here, but things change and we all
> make mistakes. This is why defensive programming

I am not totally against using default label. However, how is the following code 
is defensive?

default:
    break;

This heavily relies on the code surrounding if dealing correctly with the error 
and will never change. It feels to me we are only trying to make MISRA happy.

If we were writing defensive code, then you would definitely not have empty 
default and would at least contain an ASSERT_UNREACHABLE or error handling.

But then, you still rely on the default case to do the right thing if there was 
an error. I fail to understand how this is better than what we currently have.

>> Furthermore, how this is going to help us (thanks to -Wswitch) if an
>> enumerate is extended and we miss a case that the compiler don't notice
>> anymore?
> And if the switch's index is not an enumeration, but a plain integer...?

We have to differentiate enum vs plain integer.

For enum, a compiler supporting -Wswitch can helps us to catch when we forgot to 
handle a element of the enum. The default case defeats -Wswitch. Yet having a 
default could protect us against code error. It looks like that GCC have an 
option -Wswitch-enum that could help us. The option will similar to -Wswitch 
except it will warn even when there are a default case. I am curious to see if 
we can use it reliably in Xen.

For plain integer, we indeed need to have a *sensible* default case.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 1:23 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>> -Wswitch-default
>>>>>>>> Warn whenever a switch statement does not have a default case.
>> Will you be ok to turn this particular switch on by default?
>> Or you suggest that anyone interested to do so on their own will?
>> If we want to turn many more warnings a bit later?
>
> If we decide to fully implement the rule in Xen, then it would be nice 
> to get the compiler helping us spotting new switch without default case.
>
>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>> It is and not only in the switch. The reason I put BUG is that 
>>>>>>>> I tried
>>>>>>>> to follow
>>>>>>>> the existing "error handling" at those places.
>>>>>>> It is not because BUG() is been used today in some places that 
>>>>>>> we need to
>>>>>>> continue to spread it.
>>>>>>>
>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>> addressed
>>>>>>> So we should not add more of them...
>>>>>> Again, I see this as a dedicated change. So, in the current 
>>>>>> series I think
>>>>>> it is
>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>> That's not how it works in upstream. If you know some constructs 
>>>>> are wrong, it
>>>>> is best to try to address partially the problem directly then 
>>>>> having so you
>>>>> reduce the amounts of change afterwards.
>> So, then we need to get rid of BUG() from the existing code first,
>> not trying to solve two issues at a time: rule 16.4 and BUG().
>> I still do think these two changes are not strictly relevant,
>> but 16.4 just suffers from consequences of BUG() being used.
>
> Why would you add more BUG()? As I said above if this is an issue, 
> then you don't add more. We can deal with other later on.
>
> So let me be clearer, I will not accept any new defensive code using 
> BUG(). If you need rework in order to avoid BUG() in your series, then 
> please do it. Note, this is not a request to remove the existing BUG().
>
I am not defending BUG() in any way which is obviously a no-go.
I am just trying to say that BUG() usage in the existing code needs to be
fixed first. Once done, we can then move to "default" w/o BUG()
>>>>>
>>>>> So please try to not introduce more BUG() in the code base.
>>>> Hi Oleksandr, Julien,
>>>>
>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>> each of them makes the code less safe, not more safe! The purpose of
>>>> MISRAC 16.4 is "defensive programming": write the code in a way 
>>>> that is
>>>> more (not less!) resilient to failure.
>>>>
>>>> So, I think it is a good idea to introduce a default label because it
>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>> default handler, which is detrimental, we should return an error when
>>>> possible, or just print a warning.
>>> domain_crash() is almost always better than BUG().  It is very obvious
>>> if it gets hit, and wont crash Xen.
>> Thank you for suggestions
>>>> As 16.4 clearly state, even a simple comment would be enough to 
>>>> address
>>>> the rule. We just need to explain why a default label is not needed.
>>>> Such as:
>>>>
>>>>    default:
>>>>    /* unreachable because blah and blah */
>> This is true...
>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>> ... as well as this comment as well.
>>>
>>> This requirement is actively hostile towards compilers trying to help
>>> you spot when you made a mistake and forgot to update one of the $N
>>> places you needed to.
>> This is a trade-off: if your compiler is buggy or not supporting
>> the switches then the code itself defends from such cases, hence
>> the "default".
>
> As said before, how do you put the limit on what you need to protect 
> against? If your compiler is buggy, then probably a default in switch 
> is not going to help much.
>
This is why defensive programming was invented, no? We at least try harder
to defend ourselves from possible troubles.
Again, I am not an expert in this field. Neither I can defend MISRA and 
corresponding
requirements. But, I do believe that those rules do have some ground 
underneath...
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 1:47 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 10:11, Oleksandr Andrushchenko wrote:
>> On 2/23/19 12:08 AM, Julien Grall wrote:
>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>> -Wswitch-default
>>>> As 16.4 clearly state, even a simple comment would be enough to 
>>>> address
>>>> the rule. We just need to explain why a default label is not needed.
>>>> Such as:
>>>>
>>>>     default:
>>>>     /* unreachable because blah and blah */
>>> ... as we already have defensive code. Indeed, in most of the switch we
>>> deal with potential issue by initializing the variable before the
>>> switch. If you look at the first patch, a lot of "default: break;" is
>>> introduced. So what's our benefits? How this code is more defensive 
>>> than
>>> what we currently have?
>> ...and if you remove those initialized variables and forget
>> to update the switch with default? Or change the initial value
>> the way your previous switch assumptions do not work anymore?
>> I am not trying to defend MISRA here, but things change and we all
>> make mistakes. This is why defensive programming
>
> I am not totally against using default label. However, how is the 
> following code is defensive?
>
> default:
>    break;
>
> This heavily relies on the code surrounding if dealing correctly with 
> the error and will never change. It feels to me we are only trying to 
> make MISRA happy.
>
> If we were writing defensive code, then you would definitely not have 
> empty default and would at least contain an ASSERT_UNREACHABLE or 
> error handling.
>
> But then, you still rely on the default case to do the right thing if 
> there was an error. I fail to understand how this is better than what 
> we currently have.
>
I do agree on this. And will say it again that we are trying to work out an
acceptable approach that (if even possible) both satisfies the community by
not bringing artificial constructs like empty defaults and still satisfies
the corresponding certification requirements.
>>> Furthermore, how this is going to help us (thanks to -Wswitch) if an
>>> enumerate is extended and we miss a case that the compiler don't notice
>>> anymore?
>> And if the switch's index is not an enumeration, but a plain integer...?
>
> We have to differentiate enum vs plain integer.
>
> For enum, a compiler supporting -Wswitch can helps us to catch when we 
> forgot to handle a element of the enum. The default case defeats 
> -Wswitch. Yet having a default could protect us against code error. It 
> looks like that GCC have an option -Wswitch-enum that could help us. 
> The option will similar to -Wswitch except it will warn even when 
> there are a default case. I am curious to see if we can use it 
> reliably in Xen.
>
> For plain integer, we indeed need to have a *sensible* default case.
>
Ok, so at least with enums we can use -Wswitch-xxx approach, but I 
cannot tell
if this is enough to make MISRA happy. Probably we will need to document all
such cases, so we have a dedicated justification for every single place 
we do not want
defaults mess.
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 1:43 PM, Julien Grall wrote:
> Hi,
>
> On 22/02/2019 10:27, Andrew Cooper wrote:
>> On 22/02/2019 09:57, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Hello, everybody!
>>>
>>> We at EPAM Systems would like to present first series of patches 
>>> targeting Xen
>>> on ARM Functional Safety certification (ISO61508 based): 
>>> implementation of
>>> MISRA [1] C:2012 Rule 16.4 which requires that every switch 
>>> statement has a
>>> default label as a measure of defensive programming technique.
>>
>> Hang on - what?
>>
>> Can someone attempt to justify why actively breaking -Wswitch is going
>> to result in safer/better code?
>
> I just discovered that GCC has an option -Wswitch-enum that would warn 
> of missing element even in the presence of the 'default' label. I 
> guess this could help us to reunite MISRA and Xen for this rule?
>
And still your comment on plain integers in another thread is still valid.
So, it only partially solves the issue.
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 25.02.19 at 12:47, <julien.grall@arm.com> wrote:
> For enum, a compiler supporting -Wswitch can helps us to catch when we forgot to 
> handle a element of the enum. The default case defeats -Wswitch. Yet having a 
> default could protect us against code error. It looks like that GCC have an 
> option -Wswitch-enum that could help us. The option will similar to -Wswitch 
> except it will warn even when there are a default case. I am curious to see if 
> we can use it reliably in Xen.

I can say already now that I'm against the use of -Wswitch-enum. It
defeats the main idea of "default:" in C. The longer the list of
enumerators in an enum, the worse the situation becomes when you
mean to handle specially a couple of cases while dozens of others
use a common path, all of which would then need to get enlisted.
x86's p2m_get_iommu_flags() and hvmemul_rep_stos() may give you
a vague impression of what I mean...

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 25.02.19 at 12:49, <andr2000@gmail.com> wrote:
> I am not defending BUG() in any way which is obviously a no-go.
> I am just trying to say that BUG() usage in the existing code needs to be
> fixed first. Once done, we can then move to "default" w/o BUG()

Why? A first step to not make things worse is to not introduce
further badness. Cleaning up existing issues is an independent
(and perhaps parallel) effort.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
> On 2/25/19 1:23 PM, Julien Grall wrote:
>> Hi,
>>
>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>> -Wswitch-default
>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>> Will you be ok to turn this particular switch on by default?
>>> Or you suggest that anyone interested to do so on their own will?
>>> If we want to turn many more warnings a bit later?
>>
>> If we decide to fully implement the rule in Xen, then it would be nice to get 
>> the compiler helping us spotting new switch without default case.
>>
>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>> to follow
>>>>>>>>> the existing "error handling" at those places.
>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>> continue to spread it.
>>>>>>>>
>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>> addressed
>>>>>>>> So we should not add more of them...
>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>> it is
>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>> That's not how it works in upstream. If you know some constructs are 
>>>>>> wrong, it
>>>>>> is best to try to address partially the problem directly then having so you
>>>>>> reduce the amounts of change afterwards.
>>> So, then we need to get rid of BUG() from the existing code first,
>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>> I still do think these two changes are not strictly relevant,
>>> but 16.4 just suffers from consequences of BUG() being used.
>>
>> Why would you add more BUG()? As I said above if this is an issue, then you 
>> don't add more. We can deal with other later on.
>>
>> So let me be clearer, I will not accept any new defensive code using BUG(). If 
>> you need rework in order to avoid BUG() in your series, then please do it. 
>> Note, this is not a request to remove the existing BUG().
>>
> I am not defending BUG() in any way which is obviously a no-go.
> I am just trying to say that BUG() usage in the existing code needs to be
> fixed first. Once done, we can then move to "default" w/o BUG()

I am sorry but it does not make any sense. Why do you first need to fix the 
BUG() in the code before moving to "default" without BUG()?

>>>>>>
>>>>>> So please try to not introduce more BUG() in the code base.
>>>>> Hi Oleksandr, Julien,
>>>>>
>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>> more (not less!) resilient to failure.
>>>>>
>>>>> So, I think it is a good idea to introduce a default label because it
>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>> default handler, which is detrimental, we should return an error when
>>>>> possible, or just print a warning.
>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>> if it gets hit, and wont crash Xen.
>>> Thank you for suggestions
>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>> the rule. We just need to explain why a default label is not needed.
>>>>> Such as:
>>>>>
>>>>>    default:
>>>>>    /* unreachable because blah and blah */
>>> This is true...
>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>> ... as well as this comment as well.
>>>>
>>>> This requirement is actively hostile towards compilers trying to help
>>>> you spot when you made a mistake and forgot to update one of the $N
>>>> places you needed to.
>>> This is a trade-off: if your compiler is buggy or not supporting
>>> the switches then the code itself defends from such cases, hence
>>> the "default".
>>
>> As said before, how do you put the limit on what you need to protect against? 
>> If your compiler is buggy, then probably a default in switch is not going to 
>> help much.
>>
> This is why defensive programming was invented, no? We at least try harder
> to defend ourselves from possible troubles.

You will never be able to fully workaround a buggy compiler. Imagine the default 
case is also miscompiled? My point here is we have to

Anyway, the chance of buggy compiler is slimmer than a programming error. Hence 
the defensive programming.

> Again, I am not an expert in this field. Neither I can defend MISRA and 
> corresponding
> requirements. But, I do believe that those rules do have some ground underneath...

What I question is the way your patch implemented the rule. The goal of MISRA is 
to make the code more defensive, a few of your changes does not achieve that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 2:11 PM, Jan Beulich wrote:
>>>> On 25.02.19 at 12:49, <andr2000@gmail.com> wrote:
>> I am not defending BUG() in any way which is obviously a no-go.
>> I am just trying to say that BUG() usage in the existing code needs to be
>> fixed first. Once done, we can then move to "default" w/o BUG()
> Why? A first step to not make things worse is to not introduce
> further badness. Cleaning up existing issues is an independent
> (and perhaps parallel) effort.
That was exactly my point and I have already agreed on that
> Jan
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 2:15 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
>> On 2/25/19 1:23 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>> -Wswitch-default
>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>> Will you be ok to turn this particular switch on by default?
>>>> Or you suggest that anyone interested to do so on their own will?
>>>> If we want to turn many more warnings a bit later?
>>>
>>> If we decide to fully implement the rule in Xen, then it would be 
>>> nice to get the compiler helping us spotting new switch without 
>>> default case.
>>>
>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>> It is and not only in the switch. The reason I put BUG is 
>>>>>>>>>> that I tried
>>>>>>>>>> to follow
>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>> It is not because BUG() is been used today in some places that 
>>>>>>>>> we need to
>>>>>>>>> continue to spread it.
>>>>>>>>>
>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>> addressed
>>>>>>>>> So we should not add more of them...
>>>>>>>> Again, I see this as a dedicated change. So, in the current 
>>>>>>>> series I think
>>>>>>>> it is
>>>>>>>> acceptable to use the existing way of error handling if any at 
>>>>>>>> all.
>>>>>>> That's not how it works in upstream. If you know some constructs 
>>>>>>> are wrong, it
>>>>>>> is best to try to address partially the problem directly then 
>>>>>>> having so you
>>>>>>> reduce the amounts of change afterwards.
>>>> So, then we need to get rid of BUG() from the existing code first,
>>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>>> I still do think these two changes are not strictly relevant,
>>>> but 16.4 just suffers from consequences of BUG() being used.
>>>
>>> Why would you add more BUG()? As I said above if this is an issue, 
>>> then you don't add more. We can deal with other later on.
>>>
>>> So let me be clearer, I will not accept any new defensive code using 
>>> BUG(). If you need rework in order to avoid BUG() in your series, 
>>> then please do it. Note, this is not a request to remove the 
>>> existing BUG().
>>>
>> I am not defending BUG() in any way which is obviously a no-go.
>> I am just trying to say that BUG() usage in the existing code needs 
>> to be
>> fixed first. Once done, we can then move to "default" w/o BUG()
>
> I am sorry but it does not make any sense. Why do you first need to 
> fix the BUG() in the code before moving to "default" without BUG()?
>
This order seems to be more consistent. Otherwise, we fix consequence
and then get back to the actual cause
>>>>>>>
>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>> Hi Oleksandr, Julien,
>>>>>>
>>>>>> Julien's right that we should not introduce any more BUG()s. In 
>>>>>> fact,
>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way 
>>>>>> that is
>>>>>> more (not less!) resilient to failure.
>>>>>>
>>>>>> So, I think it is a good idea to introduce a default label 
>>>>>> because it
>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>> default handler, which is detrimental, we should return an error 
>>>>>> when
>>>>>> possible, or just print a warning.
>>>>> domain_crash() is almost always better than BUG().  It is very 
>>>>> obvious
>>>>> if it gets hit, and wont crash Xen.
>>>> Thank you for suggestions
>>>>>> As 16.4 clearly state, even a simple comment would be enough to 
>>>>>> address
>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>> Such as:
>>>>>>
>>>>>>    default:
>>>>>>    /* unreachable because blah and blah */
>>>> This is true...
>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>> ... as well as this comment as well.
>>>>>
>>>>> This requirement is actively hostile towards compilers trying to help
>>>>> you spot when you made a mistake and forgot to update one of the $N
>>>>> places you needed to.
>>>> This is a trade-off: if your compiler is buggy or not supporting
>>>> the switches then the code itself defends from such cases, hence
>>>> the "default".
>>>
>>> As said before, how do you put the limit on what you need to protect 
>>> against? If your compiler is buggy, then probably a default in 
>>> switch is not going to help much.
>>>
>> This is why defensive programming was invented, no? We at least try 
>> harder
>> to defend ourselves from possible troubles.
>
> You will never be able to fully workaround a buggy compiler. Imagine 
> the default case is also miscompiled? My point here is we have to
>
> Anyway, the chance of buggy compiler is slimmer than a programming 
> error. Hence the defensive programming.
>
>> Again, I am not an expert in this field. Neither I can defend MISRA 
>> and corresponding
>> requirements. But, I do believe that those rules do have some ground 
>> underneath...
>
> What I question is the way your patch implemented the rule. The goal 
> of MISRA is to make the code more defensive, a few of your changes 
> does not achieve that.
This is a v1 of the patch. And one of the reasons to send it as is was 
to gather
community's view on how to solve such cases.
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 12:38, Oleksandr Andrushchenko wrote:
> On 2/25/19 2:15 PM, Julien Grall wrote:
>> Hi,
>>
>> On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
>>> On 2/25/19 1:23 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>> Will you be ok to turn this particular switch on by default?
>>>>> Or you suggest that anyone interested to do so on their own will?
>>>>> If we want to turn many more warnings a bit later?
>>>>
>>>> If we decide to fully implement the rule in Xen, then it would be nice to 
>>>> get the compiler helping us spotting new switch without default case.
>>>>
>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>> to follow
>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>> It is not because BUG() is been used today in some places that we need to
>>>>>>>>>> continue to spread it.
>>>>>>>>>>
>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>> addressed
>>>>>>>>>> So we should not add more of them...
>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I think
>>>>>>>>> it is
>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>> That's not how it works in upstream. If you know some constructs are 
>>>>>>>> wrong, it
>>>>>>>> is best to try to address partially the problem directly then having so you
>>>>>>>> reduce the amounts of change afterwards.
>>>>> So, then we need to get rid of BUG() from the existing code first,
>>>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>>>> I still do think these two changes are not strictly relevant,
>>>>> but 16.4 just suffers from consequences of BUG() being used.
>>>>
>>>> Why would you add more BUG()? As I said above if this is an issue, then you 
>>>> don't add more. We can deal with other later on.
>>>>
>>>> So let me be clearer, I will not accept any new defensive code using BUG(). 
>>>> If you need rework in order to avoid BUG() in your series, then please do 
>>>> it. Note, this is not a request to remove the existing BUG().
>>>>
>>> I am not defending BUG() in any way which is obviously a no-go.
>>> I am just trying to say that BUG() usage in the existing code needs to be
>>> fixed first. Once done, we can then move to "default" w/o BUG()
>>
>> I am sorry but it does not make any sense. Why do you first need to fix the 
>> BUG() in the code before moving to "default" without BUG()?
>>
> This order seems to be more consistent. Otherwise, we fix consequence
> and then get back to the actual cause

So you will add more badness to just be consistent. Doesn't it sound not right 
for you? The more that you justify this patch series as "defensive code"...

>>>>>>>>
>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>> Hi Oleksandr, Julien,
>>>>>>>
>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>> more (not less!) resilient to failure.
>>>>>>>
>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>> possible, or just print a warning.
>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>> if it gets hit, and wont crash Xen.
>>>>> Thank you for suggestions
>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>> Such as:
>>>>>>>
>>>>>>>    default:
>>>>>>>    /* unreachable because blah and blah */
>>>>> This is true...
>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>> ... as well as this comment as well.
>>>>>>
>>>>>> This requirement is actively hostile towards compilers trying to help
>>>>>> you spot when you made a mistake and forgot to update one of the $N
>>>>>> places you needed to.
>>>>> This is a trade-off: if your compiler is buggy or not supporting
>>>>> the switches then the code itself defends from such cases, hence
>>>>> the "default".
>>>>
>>>> As said before, how do you put the limit on what you need to protect 
>>>> against? If your compiler is buggy, then probably a default in switch is not 
>>>> going to help much.
>>>>
>>> This is why defensive programming was invented, no? We at least try harder
>>> to defend ourselves from possible troubles.
>>
>> You will never be able to fully workaround a buggy compiler. Imagine the 
>> default case is also miscompiled? My point here is we have to
>>
>> Anyway, the chance of buggy compiler is slimmer than a programming error. 
>> Hence the defensive programming.
>>
>>> Again, I am not an expert in this field. Neither I can defend MISRA and 
>>> corresponding
>>> requirements. But, I do believe that those rules do have some ground 
>>> underneath...
>>
>> What I question is the way your patch implemented the rule. The goal of MISRA 
>> is to make the code more defensive, a few of your changes does not achieve that.
> This is a v1 of the patch. And one of the reasons to send it as is was to gather
> community's view on how to solve such cases.

If you are aware of potential issue, then it is custom to specify them in the 
cover letter. This would allow us to directly focus on the meat of the series 
rather than on sending an extra 40 e-mails to have more understanding.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 2:50 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 12:38, Oleksandr Andrushchenko wrote:
>> On 2/25/19 2:15 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
>>>> On 2/25/19 1:23 PM, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>>>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>> Will you be ok to turn this particular switch on by default?
>>>>>> Or you suggest that anyone interested to do so on their own will?
>>>>>> If we want to turn many more warnings a bit later?
>>>>>
>>>>> If we decide to fully implement the rule in Xen, then it would be 
>>>>> nice to get the compiler helping us spotting new switch without 
>>>>> default case.
>>>>>
>>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is 
>>>>>>>>>>>> that I tried
>>>>>>>>>>>> to follow
>>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>>> It is not because BUG() is been used today in some places 
>>>>>>>>>>> that we need to
>>>>>>>>>>> continue to spread it.
>>>>>>>>>>>
>>>>>>>>>>>> Use of BUG() itself is another topic which will also need 
>>>>>>>>>>>> to be
>>>>>>>>>>>> addressed
>>>>>>>>>>> So we should not add more of them...
>>>>>>>>>> Again, I see this as a dedicated change. So, in the current 
>>>>>>>>>> series I think
>>>>>>>>>> it is
>>>>>>>>>> acceptable to use the existing way of error handling if any 
>>>>>>>>>> at all.
>>>>>>>>> That's not how it works in upstream. If you know some 
>>>>>>>>> constructs are wrong, it
>>>>>>>>> is best to try to address partially the problem directly then 
>>>>>>>>> having so you
>>>>>>>>> reduce the amounts of change afterwards.
>>>>>> So, then we need to get rid of BUG() from the existing code first,
>>>>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>>>>> I still do think these two changes are not strictly relevant,
>>>>>> but 16.4 just suffers from consequences of BUG() being used.
>>>>>
>>>>> Why would you add more BUG()? As I said above if this is an issue, 
>>>>> then you don't add more. We can deal with other later on.
>>>>>
>>>>> So let me be clearer, I will not accept any new defensive code 
>>>>> using BUG(). If you need rework in order to avoid BUG() in your 
>>>>> series, then please do it. Note, this is not a request to remove 
>>>>> the existing BUG().
>>>>>
>>>> I am not defending BUG() in any way which is obviously a no-go.
>>>> I am just trying to say that BUG() usage in the existing code needs 
>>>> to be
>>>> fixed first. Once done, we can then move to "default" w/o BUG()
>>>
>>> I am sorry but it does not make any sense. Why do you first need to 
>>> fix the BUG() in the code before moving to "default" without BUG()?
>>>
>> This order seems to be more consistent. Otherwise, we fix consequence
>> and then get back to the actual cause
>
> So you will add more badness to just be consistent. Doesn't it sound 
> not right for you? The more that you justify this patch series as 
> "defensive code"...
>
Why? If we *first* deal with BUG() and *then* introduce "defaults" patch
which will use the already fixed code?
>>>>>>>>>
>>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>>> Hi Oleksandr, Julien,
>>>>>>>>
>>>>>>>> Julien's right that we should not introduce any more BUG()s. In 
>>>>>>>> fact,
>>>>>>>> each of them makes the code less safe, not more safe! The 
>>>>>>>> purpose of
>>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way 
>>>>>>>> that is
>>>>>>>> more (not less!) resilient to failure.
>>>>>>>>
>>>>>>>> So, I think it is a good idea to introduce a default label 
>>>>>>>> because it
>>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in 
>>>>>>>> the
>>>>>>>> default handler, which is detrimental, we should return an 
>>>>>>>> error when
>>>>>>>> possible, or just print a warning.
>>>>>>> domain_crash() is almost always better than BUG().  It is very 
>>>>>>> obvious
>>>>>>> if it gets hit, and wont crash Xen.
>>>>>> Thank you for suggestions
>>>>>>>> As 16.4 clearly state, even a simple comment would be enough to 
>>>>>>>> address
>>>>>>>> the rule. We just need to explain why a default label is not 
>>>>>>>> needed.
>>>>>>>> Such as:
>>>>>>>>
>>>>>>>>    default:
>>>>>>>>    /* unreachable because blah and blah */
>>>>>> This is true...
>>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>>> ... as well as this comment as well.
>>>>>>>
>>>>>>> This requirement is actively hostile towards compilers trying to 
>>>>>>> help
>>>>>>> you spot when you made a mistake and forgot to update one of the $N
>>>>>>> places you needed to.
>>>>>> This is a trade-off: if your compiler is buggy or not supporting
>>>>>> the switches then the code itself defends from such cases, hence
>>>>>> the "default".
>>>>>
>>>>> As said before, how do you put the limit on what you need to 
>>>>> protect against? If your compiler is buggy, then probably a 
>>>>> default in switch is not going to help much.
>>>>>
>>>> This is why defensive programming was invented, no? We at least try 
>>>> harder
>>>> to defend ourselves from possible troubles.
>>>
>>> You will never be able to fully workaround a buggy compiler. Imagine 
>>> the default case is also miscompiled? My point here is we have to
>>>
>>> Anyway, the chance of buggy compiler is slimmer than a programming 
>>> error. Hence the defensive programming.
>>>
>>>> Again, I am not an expert in this field. Neither I can defend MISRA 
>>>> and corresponding
>>>> requirements. But, I do believe that those rules do have some 
>>>> ground underneath...
>>>
>>> What I question is the way your patch implemented the rule. The goal 
>>> of MISRA is to make the code more defensive, a few of your changes 
>>> does not achieve that.
>> This is a v1 of the patch. And one of the reasons to send it as is 
>> was to gather
>> community's view on how to solve such cases.
>
> If you are aware of potential issue, then it is custom to specify them 
> in the cover letter.
My bad, but the patch itself was expected to raise questions, so I was 
not sure
what needs to be highlighted
> This would allow us to directly focus on the meat of the series 
The meat is being discussed (IMO) and it is not only empty defaults
> rather than on sending an extra 40 e-mails to have more understanding.
>
And just imagine if one spends weeks of work trying to fix some other
non-trivial requirements and then receives >40 e-mails all requesting
non-trivial changes. We already have such an example with Stefano's
series which is now v9...
So, IMO it is better to discuss some common approaches before sending
something really complex which has all chances to have v15...
And you might have noticed that the series starts with "RFC" word
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 13:06, Oleksandr Andrushchenko wrote:
> On 2/25/19 2:50 PM, Julien Grall wrote:
>> Hi,
>>
>> On 25/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>> On 2/25/19 2:15 PM, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
>>>>> On 2/25/19 1:23 PM, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>>>>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>>>> Warn whenever a switch statement does not have a default case.
>>>>>>> Will you be ok to turn this particular switch on by default?
>>>>>>> Or you suggest that anyone interested to do so on their own will?
>>>>>>> If we want to turn many more warnings a bit later?
>>>>>>
>>>>>> If we decide to fully implement the rule in Xen, then it would be nice to 
>>>>>> get the compiler helping us spotting new switch without default case.
>>>>>>
>>>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
>>>>>>>>>>>>> to follow
>>>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>>>> It is not because BUG() is been used today in some places that we 
>>>>>>>>>>>> need to
>>>>>>>>>>>> continue to spread it.
>>>>>>>>>>>>
>>>>>>>>>>>>> Use of BUG() itself is another topic which will also need to be
>>>>>>>>>>>>> addressed
>>>>>>>>>>>> So we should not add more of them...
>>>>>>>>>>> Again, I see this as a dedicated change. So, in the current series I 
>>>>>>>>>>> think
>>>>>>>>>>> it is
>>>>>>>>>>> acceptable to use the existing way of error handling if any at all.
>>>>>>>>>> That's not how it works in upstream. If you know some constructs are 
>>>>>>>>>> wrong, it
>>>>>>>>>> is best to try to address partially the problem directly then having 
>>>>>>>>>> so you
>>>>>>>>>> reduce the amounts of change afterwards.
>>>>>>> So, then we need to get rid of BUG() from the existing code first,
>>>>>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>>>>>> I still do think these two changes are not strictly relevant,
>>>>>>> but 16.4 just suffers from consequences of BUG() being used.
>>>>>>
>>>>>> Why would you add more BUG()? As I said above if this is an issue, then 
>>>>>> you don't add more. We can deal with other later on.
>>>>>>
>>>>>> So let me be clearer, I will not accept any new defensive code using 
>>>>>> BUG(). If you need rework in order to avoid BUG() in your series, then 
>>>>>> please do it. Note, this is not a request to remove the existing BUG().
>>>>>>
>>>>> I am not defending BUG() in any way which is obviously a no-go.
>>>>> I am just trying to say that BUG() usage in the existing code needs to be
>>>>> fixed first. Once done, we can then move to "default" w/o BUG()
>>>>
>>>> I am sorry but it does not make any sense. Why do you first need to fix the 
>>>> BUG() in the code before moving to "default" without BUG()?
>>>>
>>> This order seems to be more consistent. Otherwise, we fix consequence
>>> and then get back to the actual cause
>>
>> So you will add more badness to just be consistent. Doesn't it sound not right 
>> for you? The more that you justify this patch series as "defensive code"...
>>
> Why? If we *first* deal with BUG() and *then* introduce "defaults" patch
> which will use the already fixed code?
This is not how your e-mail came out. It came out as it is fine to add "default" 
with BUG() and then fix it later.

However, I don't see any strict dependencies on the both. The more than removing 
all BUG() is probably going to be more controversial than this series. So maybe 
it is good to focus on one things and do it right. Anyway, that's your call.

>>>>>>>>>>
>>>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>>>> Hi Oleksandr, Julien,
>>>>>>>>>
>>>>>>>>> Julien's right that we should not introduce any more BUG()s. In fact,
>>>>>>>>> each of them makes the code less safe, not more safe! The purpose of
>>>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
>>>>>>>>> more (not less!) resilient to failure.
>>>>>>>>>
>>>>>>>>> So, I think it is a good idea to introduce a default label because it
>>>>>>>>> can help us spot unexpected issues. Instead of calling BUG() in the
>>>>>>>>> default handler, which is detrimental, we should return an error when
>>>>>>>>> possible, or just print a warning.
>>>>>>>> domain_crash() is almost always better than BUG().  It is very obvious
>>>>>>>> if it gets hit, and wont crash Xen.
>>>>>>> Thank you for suggestions
>>>>>>>>> As 16.4 clearly state, even a simple comment would be enough to address
>>>>>>>>> the rule. We just need to explain why a default label is not needed.
>>>>>>>>> Such as:
>>>>>>>>>
>>>>>>>>>    default:
>>>>>>>>>    /* unreachable because blah and blah */
>>>>>>> This is true...
>>>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>>>> ... as well as this comment as well.
>>>>>>>>
>>>>>>>> This requirement is actively hostile towards compilers trying to help
>>>>>>>> you spot when you made a mistake and forgot to update one of the $N
>>>>>>>> places you needed to.
>>>>>>> This is a trade-off: if your compiler is buggy or not supporting
>>>>>>> the switches then the code itself defends from such cases, hence
>>>>>>> the "default".
>>>>>>
>>>>>> As said before, how do you put the limit on what you need to protect 
>>>>>> against? If your compiler is buggy, then probably a default in switch is 
>>>>>> not going to help much.
>>>>>>
>>>>> This is why defensive programming was invented, no? We at least try harder
>>>>> to defend ourselves from possible troubles.
>>>>
>>>> You will never be able to fully workaround a buggy compiler. Imagine the 
>>>> default case is also miscompiled? My point here is we have to
>>>>
>>>> Anyway, the chance of buggy compiler is slimmer than a programming error. 
>>>> Hence the defensive programming.
>>>>
>>>>> Again, I am not an expert in this field. Neither I can defend MISRA and 
>>>>> corresponding
>>>>> requirements. But, I do believe that those rules do have some ground 
>>>>> underneath...
>>>>
>>>> What I question is the way your patch implemented the rule. The goal of 
>>>> MISRA is to make the code more defensive, a few of your changes does not 
>>>> achieve that.
>>> This is a v1 of the patch. And one of the reasons to send it as is was to gather
>>> community's view on how to solve such cases.
>>
>> If you are aware of potential issue, then it is custom to specify them in the 
>> cover letter.
> My bad, but the patch itself was expected to raise questions, so I was not sure
> what needs to be highlighted
>> This would allow us to directly focus on the meat of the series 
> The meat is being discussed (IMO) and it is not only empty defaults
>> rather than on sending an extra 40 e-mails to have more understanding.
>>
> And just imagine if one spends weeks of work trying to fix some other
> non-trivial requirements and then receives >40 e-mails all requesting
> non-trivial changes. We already have such an example with Stefano's
> series which is now v9...
> So, IMO it is better to discuss some common approaches before sending
> something really complex which has all chances to have v15...
> And you might have noticed that the series starts with "RFC" word

My point is not about sending such code on the mailing list. My point is you 
need to provide as much as possible details in your cover letter so we can be 
more efficient when reviewing. For instance, many of us does not have access to 
MISRA spec because it is not free...

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 3:22 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 13:06, Oleksandr Andrushchenko wrote:
>> On 2/25/19 2:50 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 25/02/2019 12:38, Oleksandr Andrushchenko wrote:
>>>> On 2/25/19 2:15 PM, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 25/02/2019 11:49, Oleksandr Andrushchenko wrote:
>>>>>> On 2/25/19 1:23 PM, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 25/02/2019 09:50, Oleksandr Andrushchenko wrote:
>>>>>>>> On 2/22/19 11:33 PM, Andrew Cooper wrote:
>>>>>>>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
>>>>>>>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
>>>>>>>>>>>>>> BTW, I checked the series with -Wswitch-default:
>>>>>>>>>>>>>> -Wswitch-default
>>>>>>>>>>>>>> Warn whenever a switch statement does not have a default 
>>>>>>>>>>>>>> case.
>>>>>>>> Will you be ok to turn this particular switch on by default?
>>>>>>>> Or you suggest that anyone interested to do so on their own will?
>>>>>>>> If we want to turn many more warnings a bit later?
>>>>>>>
>>>>>>> If we decide to fully implement the rule in Xen, then it would 
>>>>>>> be nice to get the compiler helping us spotting new switch 
>>>>>>> without default case.
>>>>>>>
>>>>>>>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
>>>>>>>>>>>>>> It is and not only in the switch. The reason I put BUG is 
>>>>>>>>>>>>>> that I tried
>>>>>>>>>>>>>> to follow
>>>>>>>>>>>>>> the existing "error handling" at those places.
>>>>>>>>>>>>> It is not because BUG() is been used today in some places 
>>>>>>>>>>>>> that we need to
>>>>>>>>>>>>> continue to spread it.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Use of BUG() itself is another topic which will also need 
>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>> addressed
>>>>>>>>>>>>> So we should not add more of them...
>>>>>>>>>>>> Again, I see this as a dedicated change. So, in the current 
>>>>>>>>>>>> series I think
>>>>>>>>>>>> it is
>>>>>>>>>>>> acceptable to use the existing way of error handling if any 
>>>>>>>>>>>> at all.
>>>>>>>>>>> That's not how it works in upstream. If you know some 
>>>>>>>>>>> constructs are wrong, it
>>>>>>>>>>> is best to try to address partially the problem directly 
>>>>>>>>>>> then having so you
>>>>>>>>>>> reduce the amounts of change afterwards.
>>>>>>>> So, then we need to get rid of BUG() from the existing code first,
>>>>>>>> not trying to solve two issues at a time: rule 16.4 and BUG().
>>>>>>>> I still do think these two changes are not strictly relevant,
>>>>>>>> but 16.4 just suffers from consequences of BUG() being used.
>>>>>>>
>>>>>>> Why would you add more BUG()? As I said above if this is an 
>>>>>>> issue, then you don't add more. We can deal with other later on.
>>>>>>>
>>>>>>> So let me be clearer, I will not accept any new defensive code 
>>>>>>> using BUG(). If you need rework in order to avoid BUG() in your 
>>>>>>> series, then please do it. Note, this is not a request to remove 
>>>>>>> the existing BUG().
>>>>>>>
>>>>>> I am not defending BUG() in any way which is obviously a no-go.
>>>>>> I am just trying to say that BUG() usage in the existing code 
>>>>>> needs to be
>>>>>> fixed first. Once done, we can then move to "default" w/o BUG()
>>>>>
>>>>> I am sorry but it does not make any sense. Why do you first need 
>>>>> to fix the BUG() in the code before moving to "default" without 
>>>>> BUG()?
>>>>>
>>>> This order seems to be more consistent. Otherwise, we fix consequence
>>>> and then get back to the actual cause
>>>
>>> So you will add more badness to just be consistent. Doesn't it sound 
>>> not right for you? The more that you justify this patch series as 
>>> "defensive code"...
>>>
>> Why? If we *first* deal with BUG() and *then* introduce "defaults" patch
>> which will use the already fixed code?
> This is not how your e-mail came out. It came out as it is fine to add 
> "default" with BUG() and then fix it later.
Yes, you are correct. But I have an impression that couple of letters ago
I agreed that this *is* an issue and it *does need* a proper fix. And 
stated that
we might want to fix that separately. So, after that point I thought the 
sequence
will be:
1. Fix BUG()
2. Use fixed code as a basis for "defaults"
Hope we are on the same page now.
>
> However, I don't see any strict dependencies on the both. The more 
> than removing all BUG() is probably going to be more controversial 
> than this series. So maybe it is good to focus on one things and do it 
> right. Anyway, that's your call.
>
>>>>>>>>>>>
>>>>>>>>>>> So please try to not introduce more BUG() in the code base.
>>>>>>>>>> Hi Oleksandr, Julien,
>>>>>>>>>>
>>>>>>>>>> Julien's right that we should not introduce any more BUG()s. 
>>>>>>>>>> In fact,
>>>>>>>>>> each of them makes the code less safe, not more safe! The 
>>>>>>>>>> purpose of
>>>>>>>>>> MISRAC 16.4 is "defensive programming": write the code in a 
>>>>>>>>>> way that is
>>>>>>>>>> more (not less!) resilient to failure.
>>>>>>>>>>
>>>>>>>>>> So, I think it is a good idea to introduce a default label 
>>>>>>>>>> because it
>>>>>>>>>> can help us spot unexpected issues. Instead of calling BUG() 
>>>>>>>>>> in the
>>>>>>>>>> default handler, which is detrimental, we should return an 
>>>>>>>>>> error when
>>>>>>>>>> possible, or just print a warning.
>>>>>>>>> domain_crash() is almost always better than BUG().  It is very 
>>>>>>>>> obvious
>>>>>>>>> if it gets hit, and wont crash Xen.
>>>>>>>> Thank you for suggestions
>>>>>>>>>> As 16.4 clearly state, even a simple comment would be enough 
>>>>>>>>>> to address
>>>>>>>>>> the rule. We just need to explain why a default label is not 
>>>>>>>>>> needed.
>>>>>>>>>> Such as:
>>>>>>>>>>
>>>>>>>>>>    default:
>>>>>>>>>>    /* unreachable because blah and blah */
>>>>>>>> This is true...
>>>>>>>>> What a simple comment doesn't do is avoid breaking -Wswitch.
>>>>>>>> ... as well as this comment as well.
>>>>>>>>>
>>>>>>>>> This requirement is actively hostile towards compilers trying 
>>>>>>>>> to help
>>>>>>>>> you spot when you made a mistake and forgot to update one of 
>>>>>>>>> the $N
>>>>>>>>> places you needed to.
>>>>>>>> This is a trade-off: if your compiler is buggy or not supporting
>>>>>>>> the switches then the code itself defends from such cases, hence
>>>>>>>> the "default".
>>>>>>>
>>>>>>> As said before, how do you put the limit on what you need to 
>>>>>>> protect against? If your compiler is buggy, then probably a 
>>>>>>> default in switch is not going to help much.
>>>>>>>
>>>>>> This is why defensive programming was invented, no? We at least 
>>>>>> try harder
>>>>>> to defend ourselves from possible troubles.
>>>>>
>>>>> You will never be able to fully workaround a buggy compiler. 
>>>>> Imagine the default case is also miscompiled? My point here is we 
>>>>> have to
>>>>>
>>>>> Anyway, the chance of buggy compiler is slimmer than a programming 
>>>>> error. Hence the defensive programming.
>>>>>
>>>>>> Again, I am not an expert in this field. Neither I can defend 
>>>>>> MISRA and corresponding
>>>>>> requirements. But, I do believe that those rules do have some 
>>>>>> ground underneath...
>>>>>
>>>>> What I question is the way your patch implemented the rule. The 
>>>>> goal of MISRA is to make the code more defensive, a few of your 
>>>>> changes does not achieve that.
>>>> This is a v1 of the patch. And one of the reasons to send it as is 
>>>> was to gather
>>>> community's view on how to solve such cases.
>>>
>>> If you are aware of potential issue, then it is custom to specify 
>>> them in the cover letter.
>> My bad, but the patch itself was expected to raise questions, so I 
>> was not sure
>> what needs to be highlighted
>>> This would allow us to directly focus on the meat of the series 
>> The meat is being discussed (IMO) and it is not only empty defaults
>>> rather than on sending an extra 40 e-mails to have more understanding.
>>>
>> And just imagine if one spends weeks of work trying to fix some other
>> non-trivial requirements and then receives >40 e-mails all requesting
>> non-trivial changes. We already have such an example with Stefano's
>> series which is now v9...
>> So, IMO it is better to discuss some common approaches before sending
>> something really complex which has all chances to have v15...
>> And you might have noticed that the series starts with "RFC" word
>
> My point is not about sending such code on the mailing list. My point 
> is you need to provide as much as possible details in your cover 
> letter so we can be more efficient when reviewing. For instance, many 
> of us does not have access to MISRA spec because it is not free...
While I agree that one has to provide as much supporting information as 
possible
while presenting some work to the community it is that I cannot disclose
MISRA rules here. As you said, MISRA spec is not free. And of course I 
cannot
expect anyone to by it for the reason that someone wants some patch to be
"securely" or blindly reviewed. (BTW, this is the topic that has already 
been
raised in our team internally and being discussed)
>
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 25/02/2019 13:32, Oleksandr Andrushchenko wrote:
> On 2/25/19 3:22 PM, Julien Grall wrote:
>>> Why? If we *first* deal with BUG() and *then* introduce "defaults" patch
>>> which will use the already fixed code?
>> This is not how your e-mail came out. It came out as it is fine to add 
>> "default" with BUG() and then fix it later.
> Yes, you are correct. But I have an impression that couple of letters ago
> I agreed that this *is* an issue and it *does need* a proper fix. And stated that
> we might want to fix that separately. So, after that point I thought the sequence
> will be:
> 1. Fix BUG()
> 2. Use fixed code as a basis for "defaults"
> Hope we are on the same page now.
I still don't think 1. has to be fixed first. But I am happy with anything as 
long as we keep the number of BUG() added limited.

>> My point is not about sending such code on the mailing list. My point is you 
>> need to provide as much as possible details in your cover letter so we can be 
>> more efficient when reviewing. For instance, many of us does not have access 
>> to MISRA spec because it is not free...
> While I agree that one has to provide as much supporting information as possible
> while presenting some work to the community it is that I cannot disclose
> MISRA rules here. As you said, MISRA spec is not free. And of course I cannot
> expect anyone to by it for the reason that someone wants some patch to be
> "securely" or blindly reviewed. (BTW, this is the topic that has already been
> raised in our team internally and being discussed)

I understand that MISRA is not free and does not ask you to copy/paste the PDF.

What I ask is provide enough pointer for us to understand how this fits in Xen 
code base. For instance, a lot of the MISRA rules have explanation online (see 
website such as [1] and [2]). Another alternative is to summarize the issues 
with your own arguments.

Cheers,

[1] https://rules.sonarsource.com/c/RSPEC-131
[2] 
https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array

>>
>> Cheers,
>>
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 3:40 PM, Julien Grall wrote:
> Hi,
>
> On 25/02/2019 13:32, Oleksandr Andrushchenko wrote:
>> On 2/25/19 3:22 PM, Julien Grall wrote:
>>>> Why? If we *first* deal with BUG() and *then* introduce "defaults" 
>>>> patch
>>>> which will use the already fixed code?
>>> This is not how your e-mail came out. It came out as it is fine to 
>>> add "default" with BUG() and then fix it later.
>> Yes, you are correct. But I have an impression that couple of letters 
>> ago
>> I agreed that this *is* an issue and it *does need* a proper fix. And 
>> stated that
>> we might want to fix that separately. So, after that point I thought 
>> the sequence
>> will be:
>> 1. Fix BUG()
>> 2. Use fixed code as a basis for "defaults"
>> Hope we are on the same page now.
> I still don't think 1. has to be fixed first. But I am happy with 
> anything as long as we keep the number of BUG() added limited.
ok, I see
>
>
>>> My point is not about sending such code on the mailing list. My 
>>> point is you need to provide as much as possible details in your 
>>> cover letter so we can be more efficient when reviewing. For 
>>> instance, many of us does not have access to MISRA spec because it 
>>> is not free...
>> While I agree that one has to provide as much supporting information 
>> as possible
>> while presenting some work to the community it is that I cannot disclose
>> MISRA rules here. As you said, MISRA spec is not free. And of course 
>> I cannot
>> expect anyone to by it for the reason that someone wants some patch 
>> to be
>> "securely" or blindly reviewed. (BTW, this is the topic that has 
>> already been
>> raised in our team internally and being discussed)
>
> I understand that MISRA is not free and does not ask you to copy/paste 
> the PDF.
>
> What I ask is provide enough pointer for us to understand how this 
> fits in Xen code base. For instance, a lot of the MISRA rules have 
> explanation online (see website such as [1] and [2]). Another 
> alternative is to summarize the issues with your own arguments.
>
Totally agree, I'll try harder next time in finding open sources with rule's
descriptions
> Cheers,
>
> [1] https://rules.sonarsource.com/c/RSPEC-131
> [2] 
> https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array
>
>>>
>>> Cheers,
>>>
>>
>
Thank you,
Oleksandr

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 1674 bytes --]



> On 25 Feb 2019, at 13:47, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> On 2/25/19 3:40 PM, Julien Grall wrote:
>> 
>>>> My point is not about sending such code on the mailing list. My point is you need to provide as much as possible details in your cover letter so we can be more efficient when reviewing. For instance, many of us does not have access to MISRA spec because it is not free...
>>> While I agree that one has to provide as much supporting information as possible
>>> while presenting some work to the community it is that I cannot disclose
>>> MISRA rules here. As you said, MISRA spec is not free. And of course I cannot
>>> expect anyone to by it for the reason that someone wants some patch to be
>>> "securely" or blindly reviewed. (BTW, this is the topic that has already been
>>> raised in our team internally and being discussed)
>> 
>> I understand that MISRA is not free and does not ask you to copy/paste the PDF.
>> 
>> What I ask is provide enough pointer for us to understand how this fits in Xen code base. For instance, a lot of the MISRA rules have explanation online (see website such as [1] and [2]). Another alternative is to summarize the issues with your own arguments.
>> 
> Totally agree, I'll try harder next time in finding open sources with rule's
> descriptions

I am wondering, whether it would make sense to buy a set of MISRA C online copies for people who regularly review other people's code (eg. one per active committer). The cost is not that high per license
The problem is that it would exclude a part of the community
There would also be a minimal management overhead

Regards
Lars

[-- Attachment #1.2: Type: text/html, Size: 5381 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[George Dunlap]

On Mon, Feb 25, 2019 at 11:42 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 22.02.19 at 22:33, <andrew.cooper3@citrix.com> wrote:
> > P.S. There is a solution here which could work, but IMO a better use of
> > time and energy would be to get MISRA to update their rules to match
> > this century, and stop getting in the way of compiler features intended
> > to help the programmer avoid bugs.
>
> As much as I'm with you in desiring the compiler aid given to not get
> undermined, I think this MISRA rule isn't in need of modernizing: It's
> one thing for the compiler to help with in-range enumerators, and it's
> another to demand that unintentional out-of-range ones don't cause
> actual harm (like crashing your car into the next tree). This is even
> more so that iirc there's no warning if you pass a plain integer into a
> function whose parameter specifies an enum, or if you assign a plain
> integer to an enum types variable.

FWIW I was thinking the same thing.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5198 bytes --]

On Fri, 22 Feb 2019, Julien Grall wrote:
> On 22/02/2019 22:34, Stefano Stabellini wrote:
> > On Fri, 22 Feb 2019, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 22/02/2019 21:58, Stefano Stabellini wrote:
> >>> On Fri, 22 Feb 2019, Andrew Cooper wrote:
> >>>> On 22/02/2019 21:00, Stefano Stabellini wrote:
> >>>>> On Fri, 22 Feb 2019, Julien Grall wrote:
> >>>>>>>>> BTW, I checked the series with -Wswitch-default:
> >>>>>>>>> -Wswitch-default
> >>>>>>>>> Warn whenever a switch statement does not have a default case.
> >>>>>>>>>> Furthermore, using BUG() is a pretty bad idea in switch.
> >>>>>>>>> It is and not only in the switch. The reason I put BUG is that I tried
> >>>>>>>>> to follow
> >>>>>>>>> the existing "error handling" at those places.
> >>>>>>>> It is not because BUG() is been used today in some places that we need to
> >>>>>>>> continue to spread it.
> >>>>>>>>
> >>>>>>>>> Use of BUG() itself is another topic which will also need to be
> >>>>>>>>> addressed
> >>>>>>>> So we should not add more of them...
> >>>>>>> Again, I see this as a dedicated change. So, in the current series I think
> >>>>>>> it is
> >>>>>>> acceptable to use the existing way of error handling if any at all.
> >>>>>> That's not how it works in upstream. If you know some constructs are wrong, it
> >>>>>> is best to try to address partially the problem directly then having so you
> >>>>>> reduce the amounts of change afterwards.
> >>>>>>
> >>>>>> So please try to not introduce more BUG() in the code base.
> >>>>> Hi Oleksandr, Julien,
> >>>>>
> >>>>> Julien's right that we should not introduce any more BUG()s. In fact,
> >>>>> each of them makes the code less safe, not more safe! The purpose of
> >>>>> MISRAC 16.4 is "defensive programming": write the code in a way that is
> >>>>> more (not less!) resilient to failure.
> >>>>>
> >>>>> So, I think it is a good idea to introduce a default label because it
> >>>>> can help us spot unexpected issues. Instead of calling BUG() in the
> >>>>> default handler, which is detrimental, we should return an error when
> >>>>> possible, or just print a warning.
> >>>>
> >>>> domain_crash() is almost always better than BUG().  It is very obvious
> >>>> if it gets hit, and wont crash Xen.
> >>>
> >>> That's a good suggestion.
> >>>
> >>>
> >>>>> As 16.4 clearly state, even a simple comment would be enough to address
> >>>>> the rule. We just need to explain why a default label is not needed.
> >>>>> Such as:
> >>>>>
> >>>>>     default:
> >>>>>     /* unreachable because blah and blah */
> >>>>
> >>>> What a simple comment doesn't do is avoid breaking -Wswitch.
> >>>
> >>> I don't know how to reconcile 16.4 with -Wswitch. One could argue that
> >>> -Wswitch could be a good way to address 16.4, but then we introduce a
> >>> compiler specific requirement. Typically gcc is not the compiler of
> >>> choice for these environments, unfortunately forcing gcc is not an
> >>> option.
> >>
> >> Well, you could build with GCC and then build with your custom
> >> compiler...
> > 
> > This suggestion is problematic: as an individual interested in MISRA-C
> > compliance, I only have the MISRA-C rules in my hands. I don't know how
> > to deal with suggestions like this one, that don't comply to the Rules,
> > but it tries to address the same issue in a different manner.
> 
> Are you suggesting we will have to abide to all the rules even if they 
> doesn't make things worst? I was under the impression we don't necessary 
> need to follow a rule if we have justification for it.

No, I am not saying we should follow all MISRAC rules to the letter. I
am saying that one thing we need to decide is what to do with "creative
suggestions": if MISRAC tells us to do XXX because of YYY, and a
maintainer suggests to do ZZZ instead, where ZZZ kind goes in the same
direction as XXX but it is not the same.

Neither the contributor nor the maintainer actually know the impact on
safety certification of ZZZ. There is no simple way to address such a
suggestion without involving MISRAC compliance experts, and I don't know
if we want that as a community.


> > I cannot rule out that it wouldn't work, but also I cannot be sure that
> > it would work. In short, I have no way to make progress or to find out
> > how to move forward. I guess as a contributor I would be forced to go
> > back to the MISRAC compliance experts and ask for their opinion. (One
> > non-technical issue is who is going to pay them for spending their time
> > on this.) But what if they say it is not acceptable for compliance?
> > 
> > This is a great topic to discuss in March and decide what to do in these
> > situations.
> > 
> > 
> >> But, GCC is pretty much the only choice for Xen on Arm today
> >> as we don't build with clang and I pretty doubt we can build with compcert.
> > 
> > Obviously, this has to change if we want to make progress on safety
> > certifications.
> 
> I am curious to know what is plan for this. I mean that if no-one is 
> planning to make Xen build with other compilers. Then what would be the 
> benefits of this?

This is a multi-years effort, and making Xen build with other compilers
has to be one of the required work items.

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Mon, 25 Feb 2019, Lars Kurth wrote:
>       On 25 Feb 2019, at 13:47, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> On 2/25/19 3:40 PM, Julien Grall wrote:
> 
>                   My point is not about sending such code on the mailing list. My point is you need to
>                   provide as much as possible details in your cover letter so we can be more efficient
>                   when reviewing. For instance, many of us does not have access to MISRA spec because it
>                   is not free...
> 
>             While I agree that one has to provide as much supporting information as possible
>             while presenting some work to the community it is that I cannot disclose
>             MISRA rules here. As you said, MISRA spec is not free. And of course I cannot
>             expect anyone to by it for the reason that someone wants some patch to be
>             "securely" or blindly reviewed. (BTW, this is the topic that has already been
>             raised in our team internally and being discussed)
> 
> 
>       I understand that MISRA is not free and does not ask you to copy/paste the PDF.
> 
>       What I ask is provide enough pointer for us to understand how this fits in Xen code base. For instance, a lot
>       of the MISRA rules have explanation online (see website such as [1] and [2]). Another alternative is to
>       summarize the issues with your own arguments.
> 
> Totally agree, I'll try harder next time in finding open sources with rule's
> descriptions
> 
> 
> I am wondering, whether it would make sense to buy a set of MISRA C online copies for people who regularly review other people's
> code (eg. one per active committer). The cost is not that high per licenseThe problem is that it would exclude a part of the
> community
> There would also be a minimal management overhead

Excellent idea. In fact, I was discussing this with Julien last week: I
think we need all the hypervisor committers to have access to the MISRAC
spec.  Julien and I already have a copy thanks to our employers. We need
Jan, Andrew, and George to have one copy too.

Ideally, we should be able to do this in advance on the meeting next
month.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Mon, 25 Feb 2019, George Dunlap wrote:
> On Mon, Feb 25, 2019 at 11:42 AM Jan Beulich <JBeulich@suse.com> wrote:
> >
> > >>> On 22.02.19 at 22:33, <andrew.cooper3@citrix.com> wrote:
> > > P.S. There is a solution here which could work, but IMO a better use of
> > > time and energy would be to get MISRA to update their rules to match
> > > this century, and stop getting in the way of compiler features intended
> > > to help the programmer avoid bugs.
> >
> > As much as I'm with you in desiring the compiler aid given to not get
> > undermined, I think this MISRA rule isn't in need of modernizing: It's
> > one thing for the compiler to help with in-range enumerators, and it's
> > another to demand that unintentional out-of-range ones don't cause
> > actual harm (like crashing your car into the next tree). This is even
> > more so that iirc there's no warning if you pass a plain integer into a
> > function whose parameter specifies an enum, or if you assign a plain
> > integer to an enum types variable.
> 
> FWIW I was thinking the same thing.

Jan described exactly the scenario Rule 16.4 attempts to protect us
from. FYI MISRAC explains the reasons behind each rule, this is why I
think it would be great for you (you as in the x86 and Arm maintainers
mainly) to have access to it so you can read the explanation by
yourselves.

Also, all the alternative suggestions about using complier features to
check switch statements are based on the fact that one can use those
compilers. I don't think we can/should/want to mandate which compilers
are used with the Xen codebase. We don't want to get into the situation
where the code is "safe" only when it is built with gcc.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi Stefano,

On 25/02/2019 17:47, Stefano Stabellini wrote:
> On Mon, 25 Feb 2019, George Dunlap wrote:
>> On Mon, Feb 25, 2019 at 11:42 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>
>>>>>> On 22.02.19 at 22:33, <andrew.cooper3@citrix.com> wrote:
>>>> P.S. There is a solution here which could work, but IMO a better use of
>>>> time and energy would be to get MISRA to update their rules to match
>>>> this century, and stop getting in the way of compiler features intended
>>>> to help the programmer avoid bugs.
>>>
>>> As much as I'm with you in desiring the compiler aid given to not get
>>> undermined, I think this MISRA rule isn't in need of modernizing: It's
>>> one thing for the compiler to help with in-range enumerators, and it's
>>> another to demand that unintentional out-of-range ones don't cause
>>> actual harm (like crashing your car into the next tree). This is even
>>> more so that iirc there's no warning if you pass a plain integer into a
>>> function whose parameter specifies an enum, or if you assign a plain
>>> integer to an enum types variable.
>>
>> FWIW I was thinking the same thing.
> 
> Jan described exactly the scenario Rule 16.4 attempts to protect us
> from. FYI MISRAC explains the reasons behind each rule, this is why I
> think it would be great for you (you as in the x86 and Arm maintainers
> mainly) to have access to it so you can read the explanation by
> yourselves.
> 
> Also, all the alternative suggestions about using complier features to
> check switch statements are based on the fact that one can use those
> compilers. I don't think we can/should/want to mandate which compilers
> are used with the Xen codebase. We don't want to get into the situation
> where the code is "safe" only when it is built with gcc.

The compiler is not here to make it "safe", but helps you to find place where we 
forgot to handle a case when extending an enum.

Lets imagine for a moment that we decide to extend p2m_type_t. There are quite a 
few switch using this enum. You will no deny that finding out all the missing 
path during reviewing can be tricky. Here the compiler give you a bit of help to 
find to check all the places were handled.

Without the compiler aid, you may have to wait until someone hit the error path 
before actually discovering the missing bits. This may take quite sometimes 
(more than a release) depending on how much the code is exercised and 
potentially a pain to debug.

Regardless what the safety-certification folks are going to use, GCC is likely 
going to stay one of the main compiler used to build Xen. We are already 
struggling to keep up with the review, so I fail to see why we should make 
reviewer's life a bit more miserable by preventing us using "safety" options 
from GCC.

So I would like to find a way to keep that benefits for reviewers while also 
addressing MISRA rules. I suggested a GCC flag (-Wswitch-enum) on another e-mail 
[1] but Jan was not against such use.

I don't have a better suggestion so far.

Cheers,

[1] <5C73DB260200007800219CF1@prv1-mh.provo.novell.com>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Mon, 25 Feb 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/02/2019 17:47, Stefano Stabellini wrote:
> > On Mon, 25 Feb 2019, George Dunlap wrote:
> > > On Mon, Feb 25, 2019 at 11:42 AM Jan Beulich <JBeulich@suse.com> wrote:
> > > > 
> > > > > > > On 22.02.19 at 22:33, <andrew.cooper3@citrix.com> wrote:
> > > > > P.S. There is a solution here which could work, but IMO a better use
> > > > > of
> > > > > time and energy would be to get MISRA to update their rules to match
> > > > > this century, and stop getting in the way of compiler features
> > > > > intended
> > > > > to help the programmer avoid bugs.
> > > > 
> > > > As much as I'm with you in desiring the compiler aid given to not get
> > > > undermined, I think this MISRA rule isn't in need of modernizing: It's
> > > > one thing for the compiler to help with in-range enumerators, and it's
> > > > another to demand that unintentional out-of-range ones don't cause
> > > > actual harm (like crashing your car into the next tree). This is even
> > > > more so that iirc there's no warning if you pass a plain integer into a
> > > > function whose parameter specifies an enum, or if you assign a plain
> > > > integer to an enum types variable.
> > > 
> > > FWIW I was thinking the same thing.
> > 
> > Jan described exactly the scenario Rule 16.4 attempts to protect us
> > from. FYI MISRAC explains the reasons behind each rule, this is why I
> > think it would be great for you (you as in the x86 and Arm maintainers
> > mainly) to have access to it so you can read the explanation by
> > yourselves.
> > 
> > Also, all the alternative suggestions about using complier features to
> > check switch statements are based on the fact that one can use those
> > compilers. I don't think we can/should/want to mandate which compilers
> > are used with the Xen codebase. We don't want to get into the situation
> > where the code is "safe" only when it is built with gcc.
> 
> The compiler is not here to make it "safe", but helps you to find place where
> we forgot to handle a case when extending an enum.
> 
> Lets imagine for a moment that we decide to extend p2m_type_t. There are quite
> a few switch using this enum. You will no deny that finding out all the
> missing path during reviewing can be tricky. Here the compiler give you a bit
> of help to find to check all the places were handled.
> 
> Without the compiler aid, you may have to wait until someone hit the error
> path before actually discovering the missing bits. This may take quite
> sometimes (more than a release) depending on how much the code is exercised
> and potentially a pain to debug.
> 
> Regardless what the safety-certification folks are going to use, GCC is likely
> going to stay one of the main compiler used to build Xen. We are already
> struggling to keep up with the review, so I fail to see why we should make
> reviewer's life a bit more miserable by preventing us using "safety" options
> from GCC.
> 
> So I would like to find a way to keep that benefits for reviewers while also
> addressing MISRA rules. I suggested a GCC flag (-Wswitch-enum) on another
> e-mail [1] but Jan was not against such use.
> 
> I don't have a better suggestion so far.
> 
> Cheers,
> 
> [1] <5C73DB260200007800219CF1@prv1-mh.provo.novell.com>

I think it is fine to exploit compiler specific checks when available.
However, I don't think we should make any decisions on code correctness
based on the compiler checks that we introduce.

In other words, I think it is a good idea to use -Wswitch-enum if
possible, but it is not a reason for not having default labels in place
as suggested by 16.4. The code should be as correct and safe as
possible, without requiring external compiler-specific checks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 2/25/19 9:13 PM, Stefano Stabellini wrote:
> I think it is fine to exploit compiler specific checks when available.
> However, I don't think we should make any decisions on code correctness
> based on the compiler checks that we introduce.
> 
> In other words, I think it is a good idea to use -Wswitch-enum if
> possible, but it is not a reason for not having default labels in place
> as suggested by 16.4. The code should be as correct and safe as
> possible, without requiring external compiler-specific checks.

As I said on an answer to Oleksandr, I am not against of having 
"default" labels as long as they contain sensible actions.

But this does not really address my point above regarding way to help 
the review. Jan is against -Wswitch-enum, and I can understand his 
point. So how do you deal with missing item?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/25/19 11:34 PM, Julien Grall wrote:
> Hi,
>
> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>> I think it is fine to exploit compiler specific checks when available.
>> However, I don't think we should make any decisions on code correctness
>> based on the compiler checks that we introduce.
>>
>> In other words, I think it is a good idea to use -Wswitch-enum if
>> possible, but it is not a reason for not having default labels in place
>> as suggested by 16.4. The code should be as correct and safe as
>> possible, without requiring external compiler-specific checks.
>
> As I said on an answer to Oleksandr, I am not against of having 
> "default" labels as long as they contain sensible actions.
>
> But this does not really address my point above regarding way to help 
> the review. Jan is against -Wswitch-enum, and I can understand his 
> point. So how do you deal with missing item?
>
Hm, I would suggest to do the same as you would do it now: as we do not 
have -Wswitch...
in place one then add it manually, check/craft a patch and just remove 
the switch right before publishing the changes.
At the end of the day, we might want adding a compile time option which 
enables additional
compiler switches for the same purpose: this way those are disabled by 
default (so Jan will
be happy) and are enabled on request (so Julien is happy).
> Cheers,
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 25.02.19 at 18:47, <sstabellini@kernel.org> wrote:
> Also, all the alternative suggestions about using complier features to
> check switch statements are based on the fact that one can use those
> compilers. I don't think we can/should/want to mandate which compilers
> are used with the Xen codebase. We don't want to get into the situation
> where the code is "safe" only when it is built with gcc.

I'm not sure I agree, and I'm sure I don't fully agree: There are way too
many extensions we use that we are going to be able to say "Xen builds
fine with your compiler of choice". Yet building at all is broader than building
"safely".

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 25.02.19 at 22:34, <julien.grall@arm.com> wrote:
> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>> I think it is fine to exploit compiler specific checks when available.
>> However, I don't think we should make any decisions on code correctness
>> based on the compiler checks that we introduce.
>> 
>> In other words, I think it is a good idea to use -Wswitch-enum if
>> possible, but it is not a reason for not having default labels in place
>> as suggested by 16.4. The code should be as correct and safe as
>> possible, without requiring external compiler-specific checks.
> 
> As I said on an answer to Oleksandr, I am not against of having 
> "default" labels as long as they contain sensible actions.
> 
> But this does not really address my point above regarding way to help 
> the review. Jan is against -Wswitch-enum, and I can understand his 
> point. So how do you deal with missing item?

Just to clarify - I'm not entirely against the _optional_ use of
-Wswitch-enum, but the added clutter needs to either be well justified
(which so far it isn't), or conditionally hidden through some macro
(where the main purpose of the macro really is to document why such
otherwise pointless default labels are there in the first place).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/26/19 1:20 PM, Jan Beulich wrote:
>>>> On 25.02.19 at 22:34, <julien.grall@arm.com> wrote:
>> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>>> I think it is fine to exploit compiler specific checks when available.
>>> However, I don't think we should make any decisions on code correctness
>>> based on the compiler checks that we introduce.
>>>
>>> In other words, I think it is a good idea to use -Wswitch-enum if
>>> possible, but it is not a reason for not having default labels in place
>>> as suggested by 16.4. The code should be as correct and safe as
>>> possible, without requiring external compiler-specific checks.
>> As I said on an answer to Oleksandr, I am not against of having
>> "default" labels as long as they contain sensible actions.
>>
>> But this does not really address my point above regarding way to help
>> the review. Jan is against -Wswitch-enum, and I can understand his
>> point. So how do you deal with missing item?
> Just to clarify - I'm not entirely against the _optional_ use of
> -Wswitch-enum, but the added clutter needs to either be well justified
> (which so far it isn't), or conditionally hidden through some macro
> (where the main purpose of the macro really is to document why such
> otherwise pointless default labels are there in the first place).
Although I can agree that it might seem to be a good idea to hide
all those with a macro, but I am just thinking about other
cases which MISRA brings in. For example [1],
Rule 15.7 All if ... else if constructs shall be terminated with an else 
statement
So, we'll end up having lots of macros then which is obviously
not good. That said, I hope we can work out some common approach
not only to this issue, but how we deal with such in general.
>
> Jan
>
>
Thank you,
Oleksandr

[1] https://rules.sonarsource.com/c/RSPEC-126

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 26.02.19 at 12:33, <andr2000@gmail.com> wrote:
> On 2/26/19 1:20 PM, Jan Beulich wrote:
>>>>> On 25.02.19 at 22:34, <julien.grall@arm.com> wrote:
>>> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>>>> I think it is fine to exploit compiler specific checks when available.
>>>> However, I don't think we should make any decisions on code correctness
>>>> based on the compiler checks that we introduce.
>>>>
>>>> In other words, I think it is a good idea to use -Wswitch-enum if
>>>> possible, but it is not a reason for not having default labels in place
>>>> as suggested by 16.4. The code should be as correct and safe as
>>>> possible, without requiring external compiler-specific checks.
>>> As I said on an answer to Oleksandr, I am not against of having
>>> "default" labels as long as they contain sensible actions.
>>>
>>> But this does not really address my point above regarding way to help
>>> the review. Jan is against -Wswitch-enum, and I can understand his
>>> point. So how do you deal with missing item?
>> Just to clarify - I'm not entirely against the _optional_ use of
>> -Wswitch-enum, but the added clutter needs to either be well justified
>> (which so far it isn't), or conditionally hidden through some macro
>> (where the main purpose of the macro really is to document why such
>> otherwise pointless default labels are there in the first place).
> Although I can agree that it might seem to be a good idea to hide
> all those with a macro, but I am just thinking about other
> cases which MISRA brings in. For example [1],
> Rule 15.7 All if ... else if constructs shall be terminated with an else 
> statement

I very much hope that I'm not going to be the only one to refuse
to allow in any addition of such bogus "else" additions.

> So, we'll end up having lots of macros then which is obviously
> not good. That said, I hope we can work out some common approach
> not only to this issue, but how we deal with such in general.

I guess then I have to ask for giving a complete picture of what
other code uglifications are going to be proposed. If, to be MISRA-
compliant, we have to turn all of our code into an unreadable
mess, than I'm afraid I have to question whether we really want
to go that route. We then may be better off stopping the whole
exercise now, rather than after having done several initial steps
already.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Oleksandr Andrushchenko]

On 2/26/19 1:47 PM, Jan Beulich wrote:
>>>> On 26.02.19 at 12:33, <andr2000@gmail.com> wrote:
>> On 2/26/19 1:20 PM, Jan Beulich wrote:
>>>>>> On 25.02.19 at 22:34, <julien.grall@arm.com> wrote:
>>>> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>>>>> I think it is fine to exploit compiler specific checks when available.
>>>>> However, I don't think we should make any decisions on code correctness
>>>>> based on the compiler checks that we introduce.
>>>>>
>>>>> In other words, I think it is a good idea to use -Wswitch-enum if
>>>>> possible, but it is not a reason for not having default labels in place
>>>>> as suggested by 16.4. The code should be as correct and safe as
>>>>> possible, without requiring external compiler-specific checks.
>>>> As I said on an answer to Oleksandr, I am not against of having
>>>> "default" labels as long as they contain sensible actions.
>>>>
>>>> But this does not really address my point above regarding way to help
>>>> the review. Jan is against -Wswitch-enum, and I can understand his
>>>> point. So how do you deal with missing item?
>>> Just to clarify - I'm not entirely against the _optional_ use of
>>> -Wswitch-enum, but the added clutter needs to either be well justified
>>> (which so far it isn't), or conditionally hidden through some macro
>>> (where the main purpose of the macro really is to document why such
>>> otherwise pointless default labels are there in the first place).
>> Although I can agree that it might seem to be a good idea to hide
>> all those with a macro, but I am just thinking about other
>> cases which MISRA brings in. For example [1],
>> Rule 15.7 All if ... else if constructs shall be terminated with an else
>> statement
> I very much hope that I'm not going to be the only one to refuse
> to allow in any addition of such bogus "else" additions.
I don't expect such changes to be easily accepted.
>
>> So, we'll end up having lots of macros then which is obviously
>> not good. That said, I hope we can work out some common approach
>> not only to this issue, but how we deal with such in general.
> I guess then I have to ask for giving a complete picture of what
> other code uglifications are going to be proposed.
You can take a look at those rules which are partially available at [1]
>   If, to be MISRA-
> compliant, we have to turn all of our code into an unreadable
> mess,
This is why I think macros are not a good way forward
>   than I'm afraid I have to question whether we really want
> to go that route.
It depends on how you envision Xen's future and its appliances
>   We then may be better off stopping the whole
> exercise now, rather than after having done several initial steps
> already.
So, that was one of the goals of this series - to work out
approaches to solve the security certification issues
> Jan
>
>
[1] https://rules.sonarsource.com/c/RSPEC-126?search=misra

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Tue, 26 Feb 2019, Jan Beulich wrote:
> > So, we'll end up having lots of macros then which is obviously
> > not good. That said, I hope we can work out some common approach
> > not only to this issue, but how we deal with such in general.
> 
> I guess then I have to ask for giving a complete picture of what
> other code uglifications are going to be proposed. If, to be MISRA-
> compliant, we have to turn all of our code into an unreadable
> mess, than I'm afraid I have to question whether we really want
> to go that route. We then may be better off stopping the whole
> exercise now, rather than after having done several initial steps
> already.

Hi Jan,

I don't think there is a simple answer to your point. But the best way
to get an idea is to give a look at MISRA-C [1]. It's less than 50GBP, I
am hoping Lars will be able to sort it out for you. The purpose of this
work is also to provide the context for the upcoming f2f discussions.

What I am about to write needs to be verified (hopefully we'll find the
answer during the workshop) but my understanding is that even just a few
steps toward compliance are better than nothing.

Cheers,

Stefano


[1] https://www.misra.org.uk/shop/buy_now.php

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Mon, 25 Feb 2019, Julien Grall wrote:
> Hi,
> 
> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
> > I think it is fine to exploit compiler specific checks when available.
> > However, I don't think we should make any decisions on code correctness
> > based on the compiler checks that we introduce.
> > 
> > In other words, I think it is a good idea to use -Wswitch-enum if
> > possible, but it is not a reason for not having default labels in place
> > as suggested by 16.4. The code should be as correct and safe as
> > possible, without requiring external compiler-specific checks.
> 
> As I said on an answer to Oleksandr, I am not against of having "default"
> labels as long as they contain sensible actions.

Good!


> But this does not really address my point above regarding way to help the
> review. Jan is against -Wswitch-enum, and I can understand his point. So how
> do you deal with missing item?

I wonder if Coverity or PRQA can help with that.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Julien Grall]

Hi,

On 2/26/19 9:10 PM, Stefano Stabellini wrote:
> On Mon, 25 Feb 2019, Julien Grall wrote:
>> Hi,
>>
>> On 2/25/19 9:13 PM, Stefano Stabellini wrote:
>>> I think it is fine to exploit compiler specific checks when available.
>>> However, I don't think we should make any decisions on code correctness
>>> based on the compiler checks that we introduce.
>>>
>>> In other words, I think it is a good idea to use -Wswitch-enum if
>>> possible, but it is not a reason for not having default labels in place
>>> as suggested by 16.4. The code should be as correct and safe as
>>> possible, without requiring external compiler-specific checks.
>>
>> As I said on an answer to Oleksandr, I am not against of having "default"
>> labels as long as they contain sensible actions.
> 
> Good!
> 
> 
>> But this does not really address my point above regarding way to help the
>> review. Jan is against -Wswitch-enum, and I can understand his point. So how
>> do you deal with missing item?
> 
> I wonder if Coverity or PRQA can help with that.

I would be really surprised if Coverity or PRQA were able to detect a 
missing item in the enumeration if you have the 'default' label.

On a similar topic, we never reached to the point where Xen Arm is 
automatically covered by Coverity. At the moment, we rely on EPAM to 
push time to time. As we argue on getting the code safer, I think it is 
time to get this finally run by osstest.

I suspect this should be one of the step the less controversial for Xen 
community :).

Cheers,


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Lars Kurth]

> On 26 Feb 2019, at 18:27, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 26 Feb 2019, Jan Beulich wrote:
>>> So, we'll end up having lots of macros then which is obviously
>>> not good. That said, I hope we can work out some common approach
>>> not only to this issue, but how we deal with such in general.
>> 
>> I guess then I have to ask for giving a complete picture of what
>> other code uglifications are going to be proposed. If, to be MISRA-
>> compliant, we have to turn all of our code into an unreadable
>> mess, than I'm afraid I have to question whether we really want
>> to go that route. We then may be better off stopping the whole
>> exercise now, rather than after having done several initial steps
>> already.
> 
> Hi Jan,
> 
> I don't think there is a simple answer to your point. But the best way
> to get an idea is to give a look at MISRA-C [1]. It's less than 50GBP, I
> am hoping Lars will be able to sort it out for you. The purpose of this
> work is also to provide the context for the upcoming f2f discussions.


I can certainly do this: the cost of buying a few copies of MISRA C standard documents for a few committers is something I can approve without needing advisory board approval. The documents are shipped in PDF and the license is single user. To buy them I need the names and e-mail addresses of those who need it. 

I recall that I read in an earlier thread that Julien and Stefano have access to the document, which would leave Jan and a few members of Citrix staff. Can those committers who need access raise their hands? I can then go ahead and order these.

Having followed this thread (and the other MISRA related one from Stefano), it seems to me that potentially each of these discussions is quite divisive and take up a lot of discussion and emotional energy. With 143 rules and 16 "directives" (more like guidance) and some of the rules being mandatory (73%) and some advisory (27%), but the possibility to justify deviating from the rule, maybe we are approaching this wrongly. 

I have some thoughts about the approach and will follow up on this thread later today or tomorrow when I had some more time to clarify my thoughts.

Regards
Lars  


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 27.02.19 at 10:23, <lars.kurth.xen@gmail.com> wrote:

> 
>> On 26 Feb 2019, at 18:27, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> 
>> On Tue, 26 Feb 2019, Jan Beulich wrote:
>>>> So, we'll end up having lots of macros then which is obviously
>>>> not good. That said, I hope we can work out some common approach
>>>> not only to this issue, but how we deal with such in general.
>>> 
>>> I guess then I have to ask for giving a complete picture of what
>>> other code uglifications are going to be proposed. If, to be MISRA-
>>> compliant, we have to turn all of our code into an unreadable
>>> mess, than I'm afraid I have to question whether we really want
>>> to go that route. We then may be better off stopping the whole
>>> exercise now, rather than after having done several initial steps
>>> already.
>> 
>> Hi Jan,
>> 
>> I don't think there is a simple answer to your point. But the best way
>> to get an idea is to give a look at MISRA-C [1]. It's less than 50GBP, I
>> am hoping Lars will be able to sort it out for you. The purpose of this
>> work is also to provide the context for the upcoming f2f discussions.
> 
> 
> I can certainly do this: the cost of buying a few copies of MISRA C standard 
> documents for a few committers is something I can approve without needing 
> advisory board approval. The documents are shipped in PDF and the license is 
> single user. To buy them I need the names and e-mail addresses of those who 
> need it. 
> 
> I recall that I read in an earlier thread that Julien and Stefano have 
> access to the document, which would leave Jan and a few members of Citrix 
> staff. Can those committers who need access raise their hands? I can then go 
> ahead and order these.

Well, you've effectively raised my hand already. To be honest I'm not
sure I want it raised: I fear to break in tears when I would get to read
that book. In any event, I'd say ...

> Having followed this thread (and the other MISRA related one from Stefano), 
> it seems to me that potentially each of these discussions is quite divisive 
> and take up a lot of discussion and emotional energy. With 143 rules and 16 
> "directives" (more like guidance) and some of the rules being mandatory (73%) 
> and some advisory (27%), but the possibility to justify deviating from the 
> rule, maybe we are approaching this wrongly. 
> 
> I have some thoughts about the approach and will follow up on this thread 
> later today or tomorrow when I had some more time to clarify my thoughts.

... don't order anything before we aren't clear whether we really want
to do this (or even any part thereof) to the code base.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Lars Kurth]

> On 27 Feb 2019, at 10:16, Jan Beulich <jbeulich@suse.com> wrote:
> 
>>>> On 27.02.19 at 10:23, <lars.kurth.xen@gmail.com> wrote:
>> 
>> I recall that I read in an earlier thread that Julien and Stefano have 
>> access to the document, which would leave Jan and a few members of Citrix 
>> staff. Can those committers who need access raise their hands? I can then go 
>> ahead and order these.
> 
> Well, you've effectively raised my hand already. To be honest I'm not
> sure I want it raised: I fear to break in tears when I would get to read
> that book. In any event, I'd say ...

It's a reference document to look up stuff. Not something you would 
necessarily read upfront.

>> Having followed this thread (and the other MISRA related one from Stefano), 
>> it seems to me that potentially each of these discussions is quite divisive 
>> and take up a lot of discussion and emotional energy. With 143 rules and 16 
>> "directives" (more like guidance) and some of the rules being mandatory (73%) 
>> and some advisory (27%), but the possibility to justify deviating from the 
>> rule, maybe we are approaching this wrongly. 
>> 
>> I have some thoughts about the approach and will follow up on this thread 
>> later today or tomorrow when I had some more time to clarify my thoughts.
> 
> ... don't order anything before we aren't clear whether we really want
> to do this (or even any part thereof) to the code base.

Alright: firstly I need to explain that I asked EPAM to start looking a half 
dozen or so "interesting" Misra compliance issues and post RFC patches. The 
idea behind this was to gather data about how as a community we would handle 
these  kind of issues. There was a discussion about Misra (or safety related 
coding standards in general) at last years developer summit, which went nowhere 
due to lack of data. 

It is clear to me that as a community we have to deal with Misra C compliance 
and other efforts to make Xen more easily safety certifiable seriously and 
can't just wish it to go away. I think it is fair to say that the project is 
facing increased competition from KVM and containers, while at the same time 
Xen has unique advantages that lead vendors to go down the embedded/safety 
route. If, as a community we just dismiss these efforts, we risk a fork or 
those vendors going elsewhere. Neither would be good for the community.

Having seen the two discussions so far, it appears that even when we agree 
that there is an issue, we seem to have real issues agreeing on workable 
solutions. I also already had complaints that these threads generate to much 
discussion (aka "noise").

What I don't know, is whether the two issues posted (this one and 
https://markmail.org/message/ni3yziazuwb2aolx) are representative for the kind 
of issue we need to fix to achieve on Misra compliance, or whether they are 
difficult outliers.

@Oleksandr: maybe you have some insights 

So the question is how we should approach this:

1: One is to follow what we do now - post patches per issue and work through 
   them. This only really scales if the majority of patches are in essence
   uncontroversial.

2: A slightly different approach would be for EPAM to post a few more examples 
   of the type of issues that we would have to deal with if we want to be MISRA
   compliant. But that we exercise restraint in the discussion knowing that these
   are examples to inform a discussion at https://wiki.xenproject.org/wiki/
   Developer_Meeting/March2019_-_Safety_Certification and possible follow-up.

   What I was after when I asked EPAM to post Misra related patches was to
   get a sense of the impact and a sense of how easily resolvable issues are.
   But I wouldn't expect a full resolution at this stage, if there
   is controversy. 

   So maybe we can handle these in a different way. From my PoV, it would be good 
   enough if key reviewers communicated per example whether
   - They accept that fixing the issue would be beneficial
   - What concerns they have
   - And how much they would fight for or against such a patch
     (using the -2 ... +2 scale as outlined in "EXPRESSING AGREEMENT AND 
     DISAGREEMENT" in https://xenproject.org/developers/governance/#decisions
   
   Clearly there can be some discussion, but we don't really need to "fight
   to the end" over these. 

3: Or we could change approach completely and go for a more high-level
   design and/or analysis based approach before we do anything else. I will expand 
   further down.

My personal preference would be to use 2 for a few patches, followed by 
3 as it gives us a different perspective.

Let me outline my thinking on 3:

There are a few things about Misra that we do not yet fully understand on a
number of different dimensions:
a) Issues are either mandatory or advisory. The scale changes depending on 
   the required level of safety (expressed in ASIL A-D).
b) There will likely be clusters of Misra rules we likely violate frequently
   and others we are hardly or not affected by   

We should be able to pull an overview together using the QA Verify tool
maybe initially filtering out rule violations which are advisory in the context 
of the goal of achieving ASIL A/B.

We could have a discussion about these in some sort of design document which
covers the rule violations and proposes ways on how these would be addressed
maybe with some examples. This would be less labour intensive than preparing
actual patches and would keep things in one place. We can even break this into
small chunks (maybe sorted by the frequency it affects the code)

IMPORTANT:
c) There is also a provision to justify that certain Misra rules should not 
   apply for a specific code base. This is called a DEVIATION. It appears to be 
   that frequently 20-30% of rule breakages are justified away.

   However, the DEVIATIONS are typically approved by a certification body
   which decides that some justifications have merit, while others don't. The
   problem we have is that we have no idea what kind DEVIATIONS and 
   justifications we can get away with.

Again: I hope that some of this will become clearer at the safety meeting in 
March, where we will have safety certification experts and community members
working together. Maybe we can even convince one of those experts to act as
an advisor/reviewer for the project.

To do ANY of this, does however require access to the Misra rules. 

Does this make sense? 

Would you be able to follow my suggested approach?

Best Regards
Lars



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Stefano Stabellini]

On Wed, 27 Feb 2019, Lars Kurth wrote:
> > On 27 Feb 2019, at 10:16, Jan Beulich <jbeulich@suse.com> wrote:
> > 
> >>>> On 27.02.19 at 10:23, <lars.kurth.xen@gmail.com> wrote:
> >> 
> >> I recall that I read in an earlier thread that Julien and Stefano have 
> >> access to the document, which would leave Jan and a few members of Citrix 
> >> staff. Can those committers who need access raise their hands? I can then go 
> >> ahead and order these.
> > 
> > Well, you've effectively raised my hand already. To be honest I'm not
> > sure I want it raised: I fear to break in tears when I would get to read
> > that book. In any event, I'd say ...
> 
> It's a reference document to look up stuff. Not something you would 
> necessarily read upfront.
> 
> >> Having followed this thread (and the other MISRA related one from Stefano), 
> >> it seems to me that potentially each of these discussions is quite divisive 
> >> and take up a lot of discussion and emotional energy. With 143 rules and 16 
> >> "directives" (more like guidance) and some of the rules being mandatory (73%) 
> >> and some advisory (27%), but the possibility to justify deviating from the 
> >> rule, maybe we are approaching this wrongly. 
> >> 
> >> I have some thoughts about the approach and will follow up on this thread 
> >> later today or tomorrow when I had some more time to clarify my thoughts.
> > 
> > ... don't order anything before we aren't clear whether we really want
> > to do this (or even any part thereof) to the code base.
> 
> Alright: firstly I need to explain that I asked EPAM to start looking a half 
> dozen or so "interesting" Misra compliance issues and post RFC patches. The 
> idea behind this was to gather data about how as a community we would handle 
> these  kind of issues. There was a discussion about Misra (or safety related 
> coding standards in general) at last years developer summit, which went nowhere 
> due to lack of data. 
> 
> It is clear to me that as a community we have to deal with Misra C compliance 
> and other efforts to make Xen more easily safety certifiable seriously and 
> can't just wish it to go away. I think it is fair to say that the project is 
> facing increased competition from KVM and containers, while at the same time 
> Xen has unique advantages that lead vendors to go down the embedded/safety 
> route. If, as a community we just dismiss these efforts, we risk a fork or 
> those vendors going elsewhere. Neither would be good for the community.
> 
> Having seen the two discussions so far, it appears that even when we agree 
> that there is an issue, we seem to have real issues agreeing on workable 
> solutions. I also already had complaints that these threads generate to much 
> discussion (aka "noise").
> 
> What I don't know, is whether the two issues posted (this one and 
> https://markmail.org/message/ni3yziazuwb2aolx) are representative for the kind 
> of issue we need to fix to achieve on Misra compliance, or whether they are 
> difficult outliers.
> 
> @Oleksandr: maybe you have some insights 
> 
> So the question is how we should approach this:
> 
> 1: One is to follow what we do now - post patches per issue and work through 
>    them. This only really scales if the majority of patches are in essence
>    uncontroversial.
> 
> 2: A slightly different approach would be for EPAM to post a few more examples 
>    of the type of issues that we would have to deal with if we want to be MISRA
>    compliant. But that we exercise restraint in the discussion knowing that these
>    are examples to inform a discussion at https://wiki.xenproject.org/wiki/
>    Developer_Meeting/March2019_-_Safety_Certification and possible follow-up.
> 
>    What I was after when I asked EPAM to post Misra related patches was to
>    get a sense of the impact and a sense of how easily resolvable issues are.
>    But I wouldn't expect a full resolution at this stage, if there
>    is controversy. 
> 
>    So maybe we can handle these in a different way. From my PoV, it would be good 
>    enough if key reviewers communicated per example whether
>    - They accept that fixing the issue would be beneficial
>    - What concerns they have
>    - And how much they would fight for or against such a patch
>      (using the -2 ... +2 scale as outlined in "EXPRESSING AGREEMENT AND 
>      DISAGREEMENT" in https://xenproject.org/developers/governance/#decisions
>    
>    Clearly there can be some discussion, but we don't really need to "fight
>    to the end" over these. 
> 
> 3: Or we could change approach completely and go for a more high-level
>    design and/or analysis based approach before we do anything else. I will expand 
>    further down.
> 
> My personal preference would be to use 2 for a few patches, followed by 
> 3 as it gives us a different perspective.
> 
> Let me outline my thinking on 3:
> 
> There are a few things about Misra that we do not yet fully understand on a
> number of different dimensions:
> a) Issues are either mandatory or advisory. The scale changes depending on 
>    the required level of safety (expressed in ASIL A-D).
> b) There will likely be clusters of Misra rules we likely violate frequently
>    and others we are hardly or not affected by   
> 
> We should be able to pull an overview together using the QA Verify tool
> maybe initially filtering out rule violations which are advisory in the context 
> of the goal of achieving ASIL A/B.
> 
> We could have a discussion about these in some sort of design document which
> covers the rule violations and proposes ways on how these would be addressed
> maybe with some examples. This would be less labour intensive than preparing
> actual patches and would keep things in one place. We can even break this into
> small chunks (maybe sorted by the frequency it affects the code)
> 
> IMPORTANT:
> c) There is also a provision to justify that certain Misra rules should not 
>    apply for a specific code base. This is called a DEVIATION. It appears to be 
>    that frequently 20-30% of rule breakages are justified away.
> 
>    However, the DEVIATIONS are typically approved by a certification body
>    which decides that some justifications have merit, while others don't. The
>    problem we have is that we have no idea what kind DEVIATIONS and 
>    justifications we can get away with.
> 
> Again: I hope that some of this will become clearer at the safety meeting in 
> March, where we will have safety certification experts and community members
> working together. Maybe we can even convince one of those experts to act as
> an advisor/reviewer for the project.
> 
> To do ANY of this, does however require access to the Misra rules. 
> 
> Does this make sense? 
> 
> Would you be able to follow my suggested approach?

I agree with you on the suggested approaches. I would like to add that
although we are making progress with the current approach (1), it has a
very low yield, leading to very high costs to reach a positive
conclusion. It is not sustainable, neither as a community nor as
contributors/maintainers.

My second thought is that we need to have a way to deal with "creative
compliance" suggestions, such as [1]. Your idea of having a
certification expert on xen-devel could be a way to solve this.

[1] https://marc.info/?l=xen-devel&m=155111648202790 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 0/4] Add missing default labels to switch statements

[Jan Beulich]

>>> On 27.02.19 at 18:34, <lars.kurth.xen@gmail.com> wrote:
> Would you be able to follow my suggested approach?

I guess so. Some of the context should really have been provided before
the patches were posted, though. And I still think we will want to have an
overall picture of the impact on the code base, even if initially this picture
may not be fully accurate initially.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel