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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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)