> -----Original Message-----
> From: Matheus Tavares Bernardino
> <matheus.bernard...@oss.qualcomm.com>
> Sent: Monday, April 14, 2025 12:10 PM
> To: ltaylorsimp...@gmail.com
> Cc: brian.c...@oss.qualcomm.com; qemu-devel@nongnu.org;
> richard.hender...@linaro.org; phi...@linaro.org;
> matheus.bernard...@oss.qualcomm.com; a...@rev.ng; a...@rev.ng;
> marco.lie...@oss.qualcomm.com; alex.ben...@linaro.org;
> quic_mbur...@quicinc.com; sidn...@quicinc.com
> Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
>
> On Mon, 14 Apr 2025 11:19:38 -0600 <ltaylorsimp...@gmail.com> wrote:
> >
> > > -----Original Message-----
> > > From: Brian Cain <brian.c...@oss.qualcomm.com>
> > > Sent: Monday, April 7, 2025 1:27 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: brian.c...@oss.qualcomm.com; richard.hender...@linaro.org;
> > > phi...@linaro.org; matheus.bernard...@oss.qualcomm.com;
> a...@rev.ng;
> > > a...@rev.ng; marco.lie...@oss.qualcomm.com;
> > > ltaylorsimp...@gmail.com; alex.ben...@linaro.org;
> > > quic_mbur...@quicinc.com; sidn...@quicinc.com
> > > Subject: [PATCH v3 5/5] target/hexagon: Remove unreachable
> > >
> > > We should raise an exception in the event that we encounter a packet
> > > that can't be correctly decoded, not fault.
> > >
> > > Signed-off-by: Brian Cain <brian.c...@oss.qualcomm.com>
> > > ---
> > > target/hexagon/decode.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c index
> > > b5ece60450..1db7f1950f 100644
> > > --- a/target/hexagon/decode.c
> > > +++ b/target/hexagon/decode.c
> > > @@ -489,7 +489,6 @@ decode_insns(DisasContext *ctx, Insn *insn,
> > > uint32_t
> > > encoding)
> > > insn->iclass = iclass_bits(encoding);
> > > return 1;
> > > }
> > > - g_assert_not_reached();
> > > } else {
> > > uint32_t iclass = get_duplex_iclass(encoding);
> > > unsigned int slot0_subinsn = get_slot0_subinsn(encoding);
> > > @@ -512,6
> > > +511,11 @@ decode_insns(DisasContext *ctx, Insn *insn, uint32_t
> > > +encoding)
> > > }
> > > g_assert_not_reached();
> >
> > Why leave this one rather than raising an exception?
>
> Good point. I think this one should be removed as well. We have removed it
> downstream already.
>
> > > }
> > > + /*
> > > + * invalid/unrecognized opcode; return 1 and let gen_insn() raise
an
> > > + * exception when it sees this empty insn.
> > > + */
> > > + return 1;
> >
> > You should set insn->generate to NULL if you want to guarantee that
> > gen_insn will raise an exception.
>
> The caller already memset's it to 0 before passing `insn` down.
>
> > Do you have a test case for this?
>
> We do have a softmmu test for this downstream. Maybe we can adjust it for
> user-mode and upstream it with this patch.
Take a look at tests/tcg/hexagon/invalid-slots.c to see how to do this in
linux-user mode. You'll also need to modify Makefile.target in that
directory.
HTH,
Taylor
