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

[Matheus Tavares Bernardino <matheus.bernardino@oss.qualcomm.com>]

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.