Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2023-12-12 16:49, Julien Grall wrote:
> Hi,
>
> On 11/12/2023 12:32, Julien Grall wrote:
> > Hi,
> >
> > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > The branches of the switch after a call to 'do_unexpected_trap'
> > > cannot return, but there is one path that may return, hence
> > > only some clauses are marked with ASSERT_UNREACHABLE().
> > I don't understand why this is necessary. The code should never be 
> > reachable because do_unexpected_trap() is a noreturn().
> From the matrix discussion, it wasn't clear what was my position on 
> this patch.
> I would much prefer if the breaks are kept. I could accept:
>
> ASSERT_UNREACHABLE();
> break;
>
> But this solution is a Nack because if you are concerned about 
> functions like do_unexpected_trap() to return by mistaken, then it 
> needs to also be safe in production.
> The current proposal is not safe.
>
> Cheers,
Ok. I wonder whether the should be applied here in vcpreg.c:

diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
index 39aeda9dab62..089d2f03eb5e 100644
--- a/xen/arch/arm/vcpreg.c
+++ b/xen/arch/arm/vcpreg.c
@@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union 
hsr hsr)
         inject_undef_exception(regs, hsr);
         return;
     }
-
+
+    ASSERT_UNREACHABLE();
     advance_pc(regs, hsr);
 }

the rationale being that, should the switch somehow fail to return, the 
advance_pc would be called, rather than doing nothing.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)