RE: [PATCH v3 5/5] target/hexagon: Remove unreachable

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Matheus Tavares Bernardino
> <matheus.bernardino@oss.qualcomm.com>
> Sent: Monday, April 14, 2025 12:10 PM
> To: ltaylorsimpson@gmail.com
> Cc: brian.cain@oss.qualcomm.com; qemu-devel@nongnu.org;
> richard.henderson@linaro.org; philmd@linaro.org;
> matheus.bernardino@oss.qualcomm.com; ale@rev.ng; anjo@rev.ng;
> marco.liebel@oss.qualcomm.com; alex.bennee@linaro.org;
> quic_mburton@quicinc.com; sidneym@quicinc.com
> Subject: RE: [PATCH v3 5/5] target/hexagon: Remove unreachable
> 
> On Mon, 14 Apr 2025 11:19:38 -0600 <ltaylorsimpson@gmail.com> wrote:
> >
> > > -----Original Message-----
> > > From: Brian Cain <brian.cain@oss.qualcomm.com>
> > > Sent: Monday, April 7, 2025 1:27 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> > > philmd@linaro.org; matheus.bernardino@oss.qualcomm.com;
> ale@rev.ng;
> > > anjo@rev.ng; marco.liebel@oss.qualcomm.com;
> > > ltaylorsimpson@gmail.com; alex.bennee@linaro.org;
> > > quic_mburton@quicinc.com; sidneym@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.cain@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